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

When there are unknown keywords in the mdu, a warning should be given instead of them being silently dropped. #622

Open
tim-vd-aardweg opened this issue Apr 16, 2024 · 2 comments · May be fixed by #632
Assignees
Labels
priority: high type: feature Brand new functionality
Milestone

Comments

@tim-vd-aardweg
Copy link
Contributor

Is your feature request related to a problem? Please describe.
In #568 a request was made to give the user feedback on unsupported keywords instead of silently dropping them. Since the issue also requested other changes we decided to move this specific request into a separate issue.

Describe the solution you'd like
When we encounter an unknown mdu keyword, we should give a warning to the user instead of silently dropping the keyword.

@rhutten
Copy link
Collaborator

rhutten commented Apr 22, 2024

When you encounter unknown keywords. Please give the following warning: Unknown keywords are detected (in section of MDU file): list of unknown keywords. In case of unknown keywords are allowed (#630 - keywords), then the keywords will be kept in memory. (Note that these unknown keywords have no validation). Else the keywords will be dropped.

@MRVermeulenDeltares MRVermeulenDeltares linked a pull request Apr 24, 2024 that will close this issue
2 tasks
@MRVermeulenDeltares MRVermeulenDeltares linked a pull request Apr 24, 2024 that will close this issue
2 tasks
@MRVermeulenDeltares
Copy link
Contributor

MRVermeulenDeltares commented Apr 24, 2024

I have updated the changes in #632.
I have added multiple tests to verify the functioning.
Besides that I tested this to verify how it looks to the actual user during loading or setting unknown keywords.
I used the following piece of testcode to get the following output:

from pydantic import Extra
from hydrolib.core.dflowfm.mdu.models import FMModel

print("")
print("first test reading from mdu file with unknown keys:")
print("")

model = FMModel("C:\\Users\\Verme_mn\\Downloads\\with_unknown_keywords.mdu")
print("")
print("second test setting unknown keys on the model object:")
print("")

model.geometry.tim = "123"
model.geometry.tim2 = "123"
model.geometry.tim3 = "123"
model.geometry.tim ="456"

model.volumetables.tim = "123"
model.volumetables.tim2 = "123"
model.volumetables.tim3 = "123"
model.volumetables.tim ="456"

When I set the configuration to allow unknown keywords I get the following output:
Code_0SLFXQrPgP

When I set the configuration to not allow unknown keywords I get the following output:
Code_8Cm5WVpimJ

When the configuration is set to not allow unknown keywords, unknown keywords will throw an exception as before on setting with the model, e.g. model.geometry.tim = "123"

Developer opinion

@MRVermeulenDeltares:
I think we can implement the current PR #632 as is.
Since the PR will give a message based on how the configuration is set.
If the user ever adjusts this the message will be changed based on this, thus the changes can be merged seperate from the support or not support unknown keywords.
When a decision is made based on that this can be updated at that time, any keyword that is dropped should still be mentioned to the user.

Requirements from refinement

  • When a keyword is dropped, an error should be thrown instead of a message being printed.
  • The status of Config.extra will not be taken into account with throwing this error.

@MRVermeulenDeltares MRVermeulenDeltares moved this from In progress to Ready to review in HYDROLIB-core Apr 24, 2024
@MRVermeulenDeltares MRVermeulenDeltares moved this from Ready to review to In progress in HYDROLIB-core Apr 29, 2024
@MRVermeulenDeltares MRVermeulenDeltares moved this from In progress to Review/test follow up in HYDROLIB-core Apr 29, 2024
@MRVermeulenDeltares MRVermeulenDeltares moved this from Review/test follow up to In progress in HYDROLIB-core May 8, 2024
@MRVermeulenDeltares MRVermeulenDeltares moved this from In progress to Review/test follow up in HYDROLIB-core May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment