Conversation
Codecov Report
@@ Coverage Diff @@
## master #838 +/- ##
==========================================
+ Coverage 74.96% 74.99% +0.03%
==========================================
Files 42 42
Lines 3311 3315 +4
==========================================
+ Hits 2482 2486 +4
Misses 829 829
Continue to review full report at Codecov.
|
| assert e.rosdep_key == 'trusty_only_key' | ||
| assert e.os_name == 'ubuntu' | ||
| assert e.os_version == '*' | ||
| assert e.os_version == 'lucid' |
There was a problem hiding this comment.
Note that this is a change in the existing behavior, but this pattern doesn't appear anywhere in the rosdep database right now.
There was a problem hiding this comment.
this pattern doesn't appear anywhere in the rosdep database right now.
We don't have anything that fits this exactly but I think any situation where we use the combination of an os_version wildcard with specific os_version: null keys is meant to be covered by this test even if we don't have anywhere where a whole os_name is also null (as we usually just omit the key).
Am I right in thinking that this behavior change is due to 91cf3a1 which preserved the originally requested os_name and os_version?
This definitely seems like an improvement to the error messages for which a case could be made independently from the os_name wildcard support so I see it as an improvement to existing behavior. But I think that the case under test is an
There was a problem hiding this comment.
Am I right in thinking that this behavior change is due to 91cf3a1 ...?
Correct.
However, if you follow the code, the os_version isn't getting changed until L138 at which point the only possible ResolutionError would be from an explicitly nulled-out rule. Combined, this means that there is only one pattern which will behave differently under 91cf3a1:
foo:
debian:
'*': null
sid: [bar]This syntax is somewhat redundant, and causes rosdep's resolution error message to change, but not the exception type. When last I surveyed we had no occurrences of '*': null in the database, but it looks like two of them have slipped in. They should probably be removed.
An OS-level null will bypass L138 so the error message won't change. Again, though, we shouldn't have any of these in the database (even though we do).
EDIT: It looks like @clalancette requested the explicit null rules specifically, so I'd guess this is OK now and I missed the memo.
ros/rosdistro#21906 (comment)
ros/rosdistro#29316 (comment)
ros/rosdistro#27390 (comment)
So 91cf3a1 will then result in a change/improvement to the error message for the the latter two PRs.
nuclearsandwich
left a comment
nuclearsandwich
left a comment
There was a problem hiding this comment.
Looks like a solid improvement!
One thing I did notice while reviewing which isn't a blocker for this PR is that the definition test is growing towards 160 lines of asserts all with varying state. It would be nice to break that up into smaller test methods at some point.
| assert e.rosdep_key == 'trusty_only_key' | ||
| assert e.os_name == 'ubuntu' | ||
| assert e.os_version == '*' | ||
| assert e.os_version == 'lucid' |
There was a problem hiding this comment.
this pattern doesn't appear anywhere in the rosdep database right now.
We don't have anything that fits this exactly but I think any situation where we use the combination of an os_version wildcard with specific os_version: null keys is meant to be covered by this test even if we don't have anywhere where a whole os_name is also null (as we usually just omit the key).
Am I right in thinking that this behavior change is due to 91cf3a1 which preserved the originally requested os_name and os_version?
This definitely seems like an improvement to the error messages for which a case could be made independently from the os_name wildcard support so I see it as an improvement to existing behavior. But I think that the case under test is an
4 participants
This is an implementation of my proposed changes to REP 111.
The motivations and behaviors behind this implementation are discussed there.