Skip to content
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

T3D header not correct #356

Merged
merged 48 commits into from
Oct 14, 2022
Merged

T3D header not correct #356

merged 48 commits into from
Oct 14, 2022

Conversation

tim-vd-aardweg
Copy link
Contributor

Fixes #317

tim-vd-aardweg and others added 24 commits September 29, 2022 09:35
…pdated testcase data to reflect the changes made to some keywords in the .bc file.
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tim-vd-aardweg : please see my recommendations and some questions.

hydrolib/core/io/bc/models.py Show resolved Hide resolved
hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
hydrolib/core/io/bc/models.py Show resolved Hide resolved
hydrolib/core/io/bc/models.py Show resolved Hide resolved
hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@veenstrajelmer veenstrajelmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it with the testcase from #317. All reported issues are solved

@tim-vd-aardweg
Copy link
Contributor Author

Implemented review comments and fixed a small bug.

Co-authored-by: Prisca van der Sluis <36264671+priscavdsluis@users.noreply.github.com>
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will continue a separate review directly after this for Tim's later commits of this day. Those will contain important cleanups for the field name validators

"""VerticalPositionType: The vertical position type of the verticalpositions values."""

timeinterpolation: TimeInterpolation = Field(
TimeInterpolation.linear, alias="timeInterpolation"
TimeInterpolation.linear.value, alias="timeInterpolation"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can .value be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it cannot. If we leave it out here, it will write TimeInterpolation.linear to the files.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this very strange behaviour I must say 🤔

hydrolib/core/io/bc/models.py Outdated Show resolved Hide resolved
Copy link
Member

@arthurvd arthurvd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok after Prisca's earlier comments have been resolved.

@@ -462,3 +462,28 @@ def is_valid_coordinates_with_num_coordinates_specification() -> bool:
raise ValueError(error)

return root_validator(allow_reuse=True)(validate_location_specification)


def get_key_renaming_root_validator(keys_to_rename: Dict[str, List[str]]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Like this a lot :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! :)

@sonarcloud
Copy link

sonarcloud bot commented Oct 14, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
9.2% 9.2% Duplication

@priscavdsluis priscavdsluis merged commit 6b99312 into main Oct 14, 2022
@priscavdsluis priscavdsluis deleted the fix/317_t3d_header_not_correct branch October 14, 2022 08:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

t3D header not correct yet
4 participants