Fix for Issue #104 #107
Fix for Issue #104 #107
Conversation
This changes our logic for detecting duplicate definitions on case-sensitive file-systems to only operate on files of interest and to fix this detection logic per issue OpenCyphal#104. If globular searches find duplicates we do not waste time detecting this until/unless these duplicates are actually considered when parsing target definitions.
| found = list(filter(lambda d: d.full_name == full_name and d.version == version, self._lookup_definitions)) | ||
| found = list( | ||
| filter( | ||
| lambda d: d.full_name.lower() == full_name.lower() and d.version == version, self._lookup_definitions |
There was a problem hiding this comment.
This seems to make the search case-insensitive, why? In the Spec we state that identifiers are case-sensitive, but it is still considered a collision if identifiers differ by letter case only. Does this change allow the following:
# Foo.1.0.dsdl
bar.1.0 bar # Will "bar.1.0" name "Bar.1.0"?
@sealed# Bar.1.0.dsdl
@sealedIf yes, then it seems to be a problem.
There was a problem hiding this comment.
Since Bar.1.0.dsdl and bar.1.0.dsdl will both be accepted by this filter the later logic that requires only 1 result will raise the appropriate collision exception.
There was a problem hiding this comment.
Okay but I'm not talking about collisions here, but rather the selectivity of the search. If I only have Bar.1.0.dsdl and I can name it bar.1.0.dsdl, we have a problem, right?
There was a problem hiding this comment.
You don't if only 1 dsdl file is found. If two are found then it is a collision caused either by case-sensitivity issues or by multiple paths to the same definition in different files. So selectivity doesn't matter since we never select one of multiple values returned from this filter.
There was a problem hiding this comment.
This behavior is not spec-conformant. I understand that collision is a non-issue and am, in fact, talking about a completely different issue here: if the case doesn't match, the search should return nothing.
Promoting case-sensitivity issue from assertion error to DataTypeNameCollisionError
|
We good now? |
|
Ping? I have another PR parked behind this one too. |
|
Apologies for the delay, things pile up. |
Co-authored-by: Pavel Kirienko <[email protected]>
This changes our logic for detecting duplicate definitions on case-sensitive file-systems to only operate on files of interest and to fix this detection logic per issue #104.
If globular searches find duplicates we do not waste time detecting this until/unless these duplicates are actually considered when parsing target definitions.