configuration.py: add types to _whence() function, other cosmetic fixes #866
Conversation
Add typing information to the _whence() function that I found difficult to read. Other cosmetic fixes. Signed-off-by: Marc Herbert <[email protected]>
|
I struggled a bit while trying to review #849 cc: @thorsten-klein |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #866 +/- ##
=======================================
Coverage 84.52% 84.52%
=======================================
Files 11 11
Lines 3366 3366
=======================================
Hits 2845 2845
Misses 521 521
|
| def _whence(self, configfile): | ||
| if configfile == ConfigFile.ALL: | ||
| def _whence(self, locations: ConfigFile) -> list[_InternalCF | None]: | ||
| if locations == ConfigFile.ALL: |
There was a problem hiding this comment.
I think a large part of the readability issue is: ConfigFile is not a configuration file but an Enum. This is one way to mitigate this problem (without the massive churn of renaming the Enum)
There was a problem hiding this comment.
The name config for a ConfigFile object is used at many more places. It is always this enum.
So I would prefer if the naming is consistent.
Would do you think about changing the variable name (and maybe also the type name) at all places?
There was a problem hiding this comment.
I don't think local variables of a given type have to be named the same across the board.
I submitted a change in this particular place because I found it especially confusing. I had a very quick look at other places and while the name configfile is still not good, they generally did not seem as confusing. So not worth the git churn IMHO. If you find some other confusing places then by all means submit another PR.
4 participants
Add typing information to the _whence() function that I found difficult to read. Other cosmetic fixes.