-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add the concept of applicable versions #118
Conversation
Signed-off-by: hoangtungdinh <[email protected]>
Signed-off-by: hoangtungdinh <[email protected]>
7b3da21
to
67d5a95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some inconsistency in naming. Partially they are my fault, because something that should be plural was defined with singular wording.
But there should be some consistency in naming of clause / expression in version.py file.
Do not apply directly my suggestions, I had no time to test them.
if not version.is_valid_version_expression(definition_setting_expr): | ||
checker_data.result.set_checker_status( | ||
checker_bundle_name=constants.BUNDLE_NAME, | ||
checker_id=checker.CHECKER_ID, | ||
status=StatusType.ERROR, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be handled via RuleType
model
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, RuleType
accepts 1.7
as a valid definition setting, while our checks only accept 1.7.0
. Therefore, we need this condition here for now and decide later whether to enforce the complete major.minor.patch
in definition setting in rule UID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, and unfortunately this creates a little more ramifications than previously considered (during the design of the UID).
Using RuleType
we already make sure that the version is in format (\d+)(\.\d+)?(\.\d+)?
(not exactly, just for the sake of the comment), thus we can easily parse with:
r = RuleType(*args)
semver.VersionInfo(*r.definition_settings.split("."))
This has a severe limitation: 1.7
is interpreted as 1.7.0
. And this kind of parse, while correct from a semantic version point of view, it is incorrect from a semver specification point of view, where the comparison operator has an effect on the intended operation:
<2
is actually the requirement for<=1.[max].[max]
>2
is actually the requirement for>=3.0.0
The version in the uid
is 1.7
. It is referring to 1.7.0
or 1.7.[max]
. There are not enough information in the uid
itself to know, a convention shall be enforced.
So the discussion really pivotes on:
- if ASAM enforces semver for all the standards than we can enforce the full semantic versioning in rule uid
- otherwise we can forcibly set the convention that elision means minimum, thus
1.7
in uid is1.7.0
I want to raise the question to @pmai or @AsamDiegoSanchez.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting and I think we could create an issue from this discussion, @MatteoRagni.
For now, all the rule UIDs for OpenDrive are in the complete form major.minor.patch
.
If there is a rule UID defined with only major.minor
, e.g. asam.net:xodr:1.7:junctions.connection.one_connection_element
, the status of the check is explicitly set to ERROR
with the message The definition setting {rule_uid.definition_setting} is not valid.
, which I think is a good temporary compromise so that this issue does not block us from releasing.
If the input file only defines major.minor
, e.g., <header revMajor="1" revMinor="7" name="" ...
, we explicitly append the patch .0
, so 1.7
becomes 1.7.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion is resolved for the current PR, I'm not marking it as resolved to not collapse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved here
Signed-off-by: hoangtungdinh <[email protected]>
Thanks @MatteoRagni. All comments are addressed. |
Signed-off-by: hoangtungdinh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving one of the discussion as unresolved to not collapse it.
LGTM, thanks @hoangtungdinh
if not version.is_valid_version_expression(definition_setting_expr): | ||
checker_data.result.set_checker_status( | ||
checker_bundle_name=constants.BUNDLE_NAME, | ||
checker_id=checker.CHECKER_ID, | ||
status=StatusType.ERROR, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion is resolved for the current PR, I'm not marking it as resolved to not collapse it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
Signed-Off by: Diego Sanchez [email protected]
Description
Add the concept of "last supported version" as it was not supported in the rule uid. Address #117.
Main changes
asam.net:xodr:1.7.0:junctions.connection.one_connection_element
as1.7.0
. This is the only implemented rule that has the last supported version.How was the PR tested?
Notes
Related issue: #117