Skip to content

couple of small fixes and improvements to D-HyDAMO#172

Open
boyandomhof wants to merge 4 commits into
Deltares:mainfrom
boyandomhof:main
Open

couple of small fixes and improvements to D-HyDAMO#172
boyandomhof wants to merge 4 commits into
Deltares:mainfrom
boyandomhof:main

Conversation

@boyandomhof

Copy link
Copy Markdown

No description provided.

@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@rhutten

rhutten commented Jul 23, 2024

Copy link
Copy Markdown
Collaborator

@RuudHurkmans, could you review this pull request with fixes and improvements from D-HyDAMO?

@rhutten rhutten requested a review from RuudHurkmans July 23, 2024 06:42
@RuudHurkmans

Copy link
Copy Markdown
Collaborator

I agree with all the fixes and changes, however , for 90% they are also (in a nearly identical way) fixed in another pull request #181. I'm not sure whether allowing this request first causes problems in merging #181, since that contains are lot more changes. Therefore I would suggest to review and merge that first and then merge the remaining changes from this PR.

@RuudHurkmans

Copy link
Copy Markdown
Collaborator

Addition: I assigned @rhutten as an additional reviewer to #181.

@RuudHurkmans RuudHurkmans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As I said, I agree but suggest to merge the PR after #181 is merged to prevent conflicts. If you feel you prevent conflicts for #181, fine with me.

@veenstrajelmer

Copy link
Copy Markdown
Contributor

Hi @RuudHurkmans this PR is quite old and some of the changes are clearly also present in the current code. Could you please check if there is anything that would be useful to change in the current dhydamo code? If not, we can close this PR.

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.

4 participants