Accurate error message for missing configuration #670
Conversation
| conf_files = [Path(_find_in_parents(conf_dir, CONFIG_NAME)) | ||
| if _find_in_parents(conf_dir, CONFIG_NAME) | ||
| else conf_dir / CONFIG_NAME | ||
| for conf_dir in CONFIG_DIRS[::-1]] |
There was a problem hiding this comment.
Just a curiosity: why define the variablesCONFIG_NAME and CONFIG_DIRS at the very end of the file?
There was a problem hiding this comment.
Because CONFIG kind of has to be at the bottom of the file and both CONFIG_NAME and CONFIG_DIRS have one purpose: to define CONFIG. So I want them to be together, then I don't have to wonder or scroll.
| except AttributeError: | ||
| conf_files = [Path(_find_in_parents(conf_dir, CONFIG_NAME)) | ||
| if _find_in_parents(conf_dir, CONFIG_NAME) | ||
| else conf_dir / CONFIG_NAME |
There was a problem hiding this comment.
| else conf_dir / CONFIG_NAME | |
| else Path(conf_dir / CONFIG_NAME) |
Would this not be more consistent? It is not clear that _find_in_parents would return a string instead of a Path.
There was a problem hiding this comment.
🤷 conf_dir is a path though and even with the proposed change it must be one.
| def test_missing(self): | ||
| with self.assertRaises(AttributeError) as ve: | ||
| CONFIG.hazard.fire_fly.population.str() | ||
| self.assertIn("there is no 'fire_fly' configured for 'hazard'", str(ve.exception)) |
There was a problem hiding this comment.
What does one get for a message for CONFIG.random_module and for CONFIG.hazard.tropical_cyclone.some_file? Should this also be tested maybe?
There was a problem hiding this comment.
This:
- "there is no 'random_module' configured for 'climada.CONFIG' ..."
- "there is no 'some_file' configured for 'tropical_cyclone' ..."
The latter should not be tested as there is nothing such a test could contribute. The former possibly to some extent as the 'climada.CONFIG' is something special. I wouldn't but if you if you think it's worth it I can add it.
There was a problem hiding this comment.
Well, the given that there is nothing currently, but it would be possible (and in particular a user can add it right?) I would suggest adding it since it is very little extra work.
There was a problem hiding this comment.
👍 sure.
Just for understanding - what do you mean by
Well, the given that there is nothing currently, but it would be possible (and in particular a user can add it right?)
?
|
Looks good to me, thanks for the update! A few optional minor comments. |
|
Thanks @emanuel-schmid, it looks good. Fun fact: I tried to reproduce the error on #665 from |
|
What was the config value you were looking for? Perhaps it got established in the default config file ( |
Changes proposed in this PR:
climada.CONFIGan error message is provided that is clear about it and suggests a list of files that could be changed to solve the problem.This PR fixes (to some extent) #665
PR Author Checklist
develop)PR Reviewer Checklist