feat: Raise error for unknown keywords #632
Conversation
…tionManager and update notification layout for file loading
MRVermeulenDeltares
changed the title
# Conflicts: # hydrolib/core/dflowfm/ini/models.py
|
|
After the update to throw an error on an unknown key multiple test started to fail.
This is what I have found in regard to the keywords in relation with the tests, I tried to tackle all keywords but I am not 100% sure I have them all.
Action points for issue
|
tim-vd-aardweg
changed the title
|
In #634 I wanted to add this test: def test_load_model_with_research_keywords_as_fmmodel_raises_error(self):
input_mdu = (
test_input_dir / "research" / "mdu_with_research_keywords_from_dia_file_2024.03_release.mdu"
)
with pytest.raises(ValueError) as e:
_ = FMModel(filepath=input_mdu)
expected_error = "Unknown keywords are detected in section"
assert expected_error in str(e.value)But it currently fails, since we have |
| @@ -55,6 +56,15 @@ class Config: | |||
| extra = Extra.ignore | |||
| arbitrary_types_allowed = False | |||
|
|
|||
| def __init__(self, **data): | |||
| super().__init__(**data) | |||
There was a problem hiding this comment.
Wouldn't it be more efficient to first check if there are unknown keywords and then call super().__init__()? It seems we have all the data we need to determine if there are unknown keywords. And if there are unknown keywords there is no reason to let pydantic work all its magic, since we are raising an error anyway?
|
|
||
| from pydantic import Extra |
There was a problem hiding this comment.
Not used. There are a couple other imports that are not used. Please remove those as well.
| Notify the user of unknown keywords. | ||
|
|
||
| Args: | ||
| data (Dict[str, Any]) : Input data containing all set properties which are checked on unknown keywords. |
There was a problem hiding this comment.
What are set properties? It's a dict, not a set? 😊
| def __init__(self, **data): | ||
| super().__init__(**data) | ||
| self._unknown_keyword_error_manager.raise_error_for_unknown_keywords( | ||
| data, |
There was a problem hiding this comment.
It seems we are only using the keys of this dict in the raise_error_for_unknown_keywords() method. So just pass data.keys() and update the types expected by the method as you will now pass a list of strings instead of a dict.
There was a problem hiding this comment.
Nevermind, ignore this comment. It seems perfectly fine to just pass the dict :)
| self, data: Dict[str, Any], fields: Dict[str, Any], excluded_fields: Set | ||
| ) -> List[str]: | ||
| list_of_unknown_keywords = [] | ||
| for name, _ in data.items(): |
There was a problem hiding this comment.
You can use: for name in data. There is no need to get the keys and values!
| def _is_unknown_keyword( | ||
| self, name: str, fields: Dict[str, Any], excluded_fields: Set | ||
| ): | ||
| return name not in fields and name not in excluded_fields |
There was a problem hiding this comment.
In HYDROLIB-core we use pydantic to turn dictionaries into valid objects. Pydantic can map the key in the dictionary to the appropriate attribute by either looking at the field name or the field alias. This behaviour can be set in the config (config.allow_population_by_field_name). In our BaseModel we set config.allow_population_by_field_name to True. This means that we want to be able to map the data via either the name or the alias.
In your implementation, we say that a keyword is unknown if there is no attribute with the same name as the key. However, this will, for example, fail for the research keywords that are implemented in #642. All research keyword attribute names are prefixed with research. This will fail the mapping and they will be considered an unknown keyword. I therefore think that it is important to not only check the field names, but also their alias!
| @@ -55,6 +56,15 @@ class Config: | |||
| extra = Extra.ignore | |||
| arbitrary_types_allowed = False | |||
|
|
|||
| def __init__(self, **data): | |||
There was a problem hiding this comment.
Was there a reason we could not put this in a root_validator?
|
This doesn't work for unknown sections, right? Should you create a follow-up issue for this? |
| @@ -388,3 +392,115 @@ def test_loading_fmmodel_model_with_both_ini_and_xyn_obsfiles(self): | |||
| assert actual_point.x == expected_point.x | |||
| assert actual_point.y == expected_point.y | |||
| assert actual_point.name == expected_point.name | |||
|
|
|||
| def test_mdu_unknown_keywords_loading_gives_message_for_missing_keyword( | |||
There was a problem hiding this comment.
| def test_mdu_unknown_keywords_loading_gives_message_for_missing_keyword( | |
| def test_mdu_unknown_keywords_loading_gives_message_for_unknown_keyword( |
| tmp_mdu_path.write_text(tmp_mdu) | ||
|
|
||
| with patch( | ||
| "hydrolib.core.dflowfm.ini.models.INIBasedModel.Config" |
There was a problem hiding this comment.
Why do we need this mock?
| ) | ||
| assert name in captured.out | ||
|
|
||
| def test_mdu_unknown_keywords_loading_gives_message_for_missing_keyword2( |
There was a problem hiding this comment.
Give it an appropriate name please
| FMModel(filepath=tmp_mdu_path) | ||
| captured = capsys.readouterr() | ||
|
|
||
| excluded_fields = ["comments", "datablock", "_header"] |
There was a problem hiding this comment.
This test will still pass if we ever change the _exclude_fields of the model. So maybe use: excluded_fields = model._exclude_fields instead? Maybe even assert that the list is not empty?
| for excluded_field in excluded_fields: | ||
| assert excluded_field not in captured.out | ||
|
|
||
| def test_mdu_unknown_keywords_allow_extra_setting_field_gives_message(self, capsys): |
There was a problem hiding this comment.
In my opinion there is no need to check all the different options for Extra.<allow/forbid/ignore>. The user should not edit the values we have set, since it's not a public setting. If they decide to change it anyway, we can't offer support/validation.
#622