-
Notifications
You must be signed in to change notification settings - Fork 4
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 Version checks to all relevant models. #180
base: main
Are you sure you want to change the base?
Conversation
@arthurvd Downside to this work is that we now write the lower bound version number to files. |
Kudos, SonarCloud Quality Gate passed! |
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.
Hey @evetion , I've added some comments, but I am in doubt about whether we should squeeze this into the 0.1.6 release, as I think it deserves some further thinking.
Writing "1.09" FM-style version as `FMVersion("1.9.0") is pretty confusing. Maybe we should consider constructors using major, minor as indifvidual arguments.
Anyways, food for thought.
@@ -69,3 +72,55 @@ def get_substring_between(source: str, start: str, end: str) -> Optional[str]: | |||
return None | |||
|
|||
return source[index_start:index_end] | |||
|
|||
|
|||
class HVersion(SemVer): |
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.
Docstrings for the purpose of these classes HVersion
, etc., and subroutines such as coerce()
would help understanding a lot.
return cls(**version.__dict__) | ||
|
||
|
||
class FMVersion(HVersion): |
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 don't quite like this FM-related class here, inside a "neutral" utils namespace.
Also it suggest that this is "the" FM version, whereas it is only the numbering schema, so consider renaming this class to: class FMStyleVersion
, or DHydroStyleVersion
, or ZeroPrefixedVersion
.
|
||
|
||
class General(INIGeneral): | ||
_header: Literal["General"] = "General" | ||
program: str = Field("D-Flow FM", alias="program") | ||
version: str = Field("1.2.94.66079M", alias="version") | ||
filetype: Literal["modelDef"] = Field("modelDef", alias="fileType") | ||
fileversion: str = Field("1.09", alias="fileVersion") | ||
fileversion: FMVersion = Field(FMVersion("1.9.0"), alias="fileVersion") |
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 understand the attempt, yet I find it reads quite confusing.
Agreed, while most of the comments can be easily resolved, it's most about the lower/upper bound discussion (what do we write as version) that needs some thought. |
And lowered bounds of default versions.
Fixes #94.