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, an error should be given instead of them being silently dropped. #622

Closed
tim-vd-aardweg opened this issue Apr 16, 2024 · 2 comments · Fixed by #632
Assignees
Labels
priority: high type: compatibility Changes needed to be compatible with the computational core type: feature Brand new functionality
Milestone

Comments

@tim-vd-aardweg
Copy link
Contributor

tim-vd-aardweg commented Apr 16, 2024

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 an error 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
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.

@tim-vd-aardweg tim-vd-aardweg changed the title When there are unknown keywords in the mdu, a warning should be given instead of them being silently dropped. When there are unknown keywords in the mdu, an error should be given instead of them being silently dropped. May 30, 2024
@veenstrajelmer veenstrajelmer self-assigned this Jun 7, 2024
tim-vd-aardweg pushed a commit that referenced this issue Jun 7, 2024
@priscavdsluis priscavdsluis added the type: compatibility Changes needed to be compatible with the computational core label Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment