Use regex-based version casting that accept #658
Conversation
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
| @@ -34,6 +34,7 @@ class Issues: | |||
| We use sets to represent errors and warnings to avoid repeated strings. | |||
| """ | |||
|
|
|||
| ignore_warnings: bool = False | |||
There was a problem hiding this comment.
I think it belongs here, but if you see it more fit it could also be an argument of report()
| @@ -34,6 +34,7 @@ class Issues: | |||
| We use sets to represent errors and warnings to avoid repeated strings. | |||
| """ | |||
|
|
|||
| ignore_warnings: bool = False | |||
| issues.add_error("foo", metadata) | ||
| issues.add_warning("bar", file_object) | ||
|
|
||
| print(issues.report()) |
|
That's right, this would actually address the "However, adding a generic --ignore framework is probably a good first step, so I'm opening this issue." part of the issue, but I think a more long-term solution might be needed to silence only a subset of the warnings. |
|
Updated the PR as discussed offline. |
| raise WarningException( | ||
| f"Version doesn't follow MAJOR.MINOR.PATCH: {version}. For more" | ||
| " information refer to: https://semver.org/spec/v2.0.0.html" | ||
| f"Version contains non-numeric characters: {version}." |
There was a problem hiding this comment.
Keeping the warning for non-numeric versioning. Reasonable? Or shall I remove this as well?
There was a problem hiding this comment.
We could fully implement the semantic versioning standard.
semver is a python library that does exactly this. However it seems we only need to check the compatibility with the standard, so we could also just reuse their regular expression.
There was a problem hiding this comment.
Also, I think that it's worth mentioning the semantic versioning standard: https://semver.org/spec/v2.0.0.html, and that minor and patch can be omitted.
|
@marcenacp updated the PR -- PTAL, thanks! :) |
| @@ -101,17 +101,9 @@ def test_invalid_version(version, expected_error): | |||
| @pytest.mark.parametrize( | |||
| ["version", "expected_warning"], | |||
| [ | |||
| [ | |||
There was a problem hiding this comment.
Above, you could add that the versions 1 and 1.2 now succeed
ccl-core
changed the title
Remove warning when version doesn't conform to MAJOR.MINOR.PATCH
Ref. ##609