Use a stricter semantic version check for some version fields #225

Merged
Dalai Felinto merged 4 commits from fix-semver-versions into main 2024-07-23 15:50:54 +02:00

This applies to:

  • blender_version_min
  • blender_version_max
  • schema_version

The only field which still takes a full semantic version is the
version field itself.

See related discussion:
blender/blender#124885

This applies to: * blender_version_min * blender_version_max * schema_version The only field which still takes a full semantic version is the `version` field itself. See related discussion: https://projects.blender.org/blender/blender/issues/124885
Dalai Felinto added 1 commit 2024-07-23 13:00:42 +02:00
This applies to:
* blender_version_min
* blender_version_max
* schema_version

The only field which still takes a full semantic version is the
`version` field itself.

See related discussion:
blender/blender#124885
Oleg-Komarov requested changes 2024-07-23 13:22:25 +02:00
Dismissed
@ -537,3 +537,3 @@
class SchemaVersionValidator(VersionValidator):
class NonSemVerVersionValidator(VersionValidator):
Owner

this is a confusing name, especially when the docstring says that it is a semantic version.

a better name may be SemVerNoLabelValidator, or something similar

this is a confusing name, especially when the docstring says that it **is** a semantic version. a better name may be `SemVerNoLabelValidator`, or something similar
dfelinto marked this conversation as resolved
@ -540,0 +544,4 @@
@staticmethod
def is_version_string(version):
import re
Owner

this import should be at the top of the file

this import should be at the top of the file
dfelinto marked this conversation as resolved
@ -540,0 +550,4 @@
# https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
# Define the regex pattern for major.minor.patch (do not include any additional label).
pattern = r'^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)'
Owner

missing $ anchor

I would also declare this as a package "constant" at the top of the file, so that it is compiled once, and not on every validation

missing `$` anchor I would also declare this as a package "constant" at the top of the file, so that it is compiled once, and not on every validation
dfelinto marked this conversation as resolved
@ -540,0 +553,4 @@
pattern = r'^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)'
# Make sure we match the entire string with the pattern.
return bool(re.fullmatch(pattern, version))
Owner

this is unusual, why not use the $ anchor in the regex and do a regular match?

this is unusual, why not use the `$` anchor in the regex and do a regular match?
dfelinto marked this conversation as resolved
@ -540,0 +567,4 @@
# Play safe and first guarantee that the string can be validated as a Version type
# which is expected throghout the rest of the code.
if super().validate(name=name, value=value, manifest=manifest):
Owner

this is not needed, we need to fully rely on the check below

this is not needed, we need to fully rely on the check below
dfelinto marked this conversation as resolved
Dalai Felinto added 3 commits 2024-07-23 14:32:32 +02:00
Author
Owner

Ready for another round

Ready for another round
Oleg-Komarov approved these changes 2024-07-23 14:49:43 +02:00
Oleg-Komarov left a comment
Owner

looks good!

looks good!
Dalai Felinto merged commit b7d39288ae into main 2024-07-23 15:50:54 +02:00
Dalai Felinto deleted branch fix-semver-versions 2024-07-23 15:50:55 +02:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: infrastructure/extensions-website#225
No description provided.