Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/correct misleading qos overwrite warning #1806

Conversation

Hugal31
Copy link
Contributor

@Hugal31 Hugal31 commented Feb 28, 2025

Refactor PublisherBuilder::apply_qos_overwrites and only shows the warning if multiple nodes with overwrites matches the publisher.

Improve the warning to show all matching key expressions.

Fixes #1805

Copy link

PR missing one of the required labels: {'bug', 'documentation', 'new feature', 'dependencies', 'internal', 'breaking-change', 'enhancement'}

@Hugal31 Hugal31 force-pushed the fix/correct-misleading-qos-overwrite-warning branch from 607be05 to 087636c Compare February 28, 2025 18:52
Copy link

PR missing one of the required labels: {'breaking-change', 'internal', 'documentation', 'bug', 'new feature', 'dependencies', 'enhancement'}

Copy link
Contributor

@oteffahi oteffahi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address nitpick comment. Other than that it LGTM.

}
break;
if let Some(node) = nodes_including.first() {
qos_overwrites = node.weight().unwrap().clone();
Copy link
Contributor

@oteffahi oteffahi Mar 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace the unwrap with expect("first node weight should not be None")

@oteffahi oteffahi added the bug Something isn't working label Mar 3, 2025
@Hugal31 Hugal31 force-pushed the fix/correct-misleading-qos-overwrite-warning branch from 4ebe2fd to 4441dab Compare March 5, 2025 10:23
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 58.33333% with 5 lines in your changes missing coverage. Please review.

Project coverage is 70.34%. Comparing base (ac19c92) to head (4441dab).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
zenoh/src/api/builders/publisher.rs 58.33% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
+ Coverage   70.22%   70.34%   +0.11%     
==========================================
  Files         358      358              
  Lines       64584    64586       +2     
==========================================
+ Hits        45357    45433      +76     
+ Misses      19227    19153      -74     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fuzzypixelz fuzzypixelz merged commit cac67d9 into eclipse-zenoh:main Mar 5, 2025
15 of 16 checks passed
zettascale-bot pushed a commit to ZettaScaleLabs/zenoh that referenced this pull request Mar 5, 2025
* Filter on actually overwriting nodes when applying QoS overwrites

* Refactor and improve warning message about multiple QoS overwrites matching one publisher

---------

Co-authored-by: Hugo Laloge <hugolaloge@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading warning for qos overwrites starting with **/
3 participants