[internal] Somewhat simplify config parsing #14699
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Simplify Config.get to always return a string. It was being used that way in practice everywhere within the options system (which does the type conversion later). The only place Config.get was used for type conversion was in the special case of cli.alias, which the options bootstrapper reads directly from config. So the conversion in that one case was inlined. As a byproduct, gets rid of the default= arg to Config.get, and always returns None if the value doesn't exist, since the default= arg was, again, only used in that one special case. - Fix the type signature of get_value, which can never in fact return None. - Use our own exception types instead of borrowing the ones from configparser, now that ini parsing is a distant memory. This makes the Config class easier to reason about. [ci skip-rust] [ci skip-build-wheels]
|
This is pre-work to make it simpler to tackle #14679 |
Eric-Arellano
approved these changes
Mar 4, 2022
There was a problem hiding this comment.
Yay! Bye bye configparser.
If it helps at all, I think it's very reasonable to get rid of our abstract class for Config vs our Toml implementation. It seems very very unlikely we'll want to support more config file formats. Right now there's over-generalization, like the difference from .get() vs .get_value() is unnecessary.
| def get(self, section, option, type_=str, default=None): | ||
| """Retrieves option from the specified section (or 'DEFAULT') and attempts to parse it as | ||
| type. | ||
| def get(self, section, option) -> str | None: |
There was a problem hiding this comment.
This should probably have type hints too:
Suggested change
| def get(self, section, option) -> str | None: | |
| def get(self, section: str, option: str) -> str | None: |
There was a problem hiding this comment.
Fine to do in followup
|
Btw this should be |
benjyw
changed the title
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Simplify Config.get to always return a string.
It was being used that way in practice everywhere within
the options system (which does the type conversion later).
The only place Config.get was used for type conversion was
in the special case of cli.alias, which the options bootstrapper
reads directly from config. So the conversion in that one
case was inlined.
As a byproduct, gets rid of the default= arg to Config.get,
and always returns None if the value doesn't exist, since
the default= arg was, again, only used in that one special case.
Fix the type signature of get_value, which can never
in fact return None.
Use our own exception types and constants, instead of
borrowing the ones from configparser, now that ini parsing is
a distant memory.
This makes the Config class easier to reason about.
[ci skip-rust]
[ci skip-build-wheels]