-
Notifications
You must be signed in to change notification settings - Fork 1
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
Improve config #54
base: main
Are you sure you want to change the base?
Improve config #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this improvement with this clever way to handle the NO_CHANGE
case. Before tackling my other review comments here, I think we should discuss again if we really want to settle on the API you have introduced via this PR as it differs from my initial suggestions in #36.
I currently see a problem with this usage example:
# Current usage example
wiz.config(sigfigs=2) # explicitly set sigfigs value to 2
wiz.config(identifier="result") # sigfigs value does not change and stays 2
wiz.config(sigfigs=None, identifier=None) # reset sigfigs to default value
wiz.config(sigfigs=-1) # explicitly set sigfigs value to -1
I don't think that having values like -1
for sigfigs
is a good thing to do. In this case, the -1
is supposed to indicate that we don't want to use explicit sigfigs and instead use the predefined rounding rules. But I think it was a shortcoming in our previous design that we used -1
for this and not None
. In my opinion, in a Python world it's way more natural to write wiz.config(sigfigs=None)
if you want that sigfigs
should not have effects, i.e. reserve None
for an actual value-related meaning (and not "reset to default").
But then, this would clash with your changes in this PR where you reserve None
for the case reset to default value. I'd like to make more explicit to the caller that they reset a value, e.g. by using the name "reset" or "default" somewhere. Here are a few proposals:
# No separate reset() method (currently implemented in this PR)
wiz.config(sigfigs=None, identifier=None)
# Separate reset() method with True parameters
wiz.reset(sigfigs=True, identifier=True)
# Separate reset() method with String parameters
# We would still get IntelliSense via the IDE by using Literal["sigfigs", "identifier"] as type hint
wiz.reset("sigfigs", "identifier")
# More fluent API
wiz.config.sigfigs.set(2)
wiz.config.sigfigs.reset()
# Advantage: almost like an English sentence
# Big disadvantage: cannot set/reset multiple values at the same time
# Enums
wiz.config(sigfigs=2)
wiz.reset(wiz.config.SIGFIGS, wiz.config.DECIMAL_PLACES)
# Enums other idea without separate reset() method
wiz.config(sigfigs=2)
wiz.config(sigfigs=wiz.config.DEFAULT)
# Allow separate TOML-file for configuration?
# wiz.config.from_file("config.toml")
# Probably don't want that as this would mean one extra file for the user
# to setup and there are not THAT many configuration options in our library.
I currently like wiz.reset(sigfigs=True, identifier=True)
and wiz.config(sigfigs=wiz.config.DEFAULT)
the most. I would have loved to write wiz.reset(sigfigs, identifier)
, but that's not possible in Python. Maybe you can also research a bit to come up with more proposals to choose from. Maybe you have some strong opinions on this? My only strong opinion here is that we should rethink if we really want None
to indicate Default
. As I said, we should reserve it for cases like sigfigs
(or other options) where it has an actual meaning related to the value itself. And yes, these questions are kind of philosophical, but that's probably the point of API design ;)
Edit: This StackOverflow answer might be related.
Edit2: When discussing different options, I think we should focus on how easy the API is to use for the user, and not on how difficult it might be to implement.
Edit3: The Python documentation for None
just says that None
is "frequently used to represent the absence of a value, as when default arguments are not passed to a function".
Edit4: I've implemented a quick preview of how the API could look like with the wiz.config(sigfigs=wiz.CONFIG_DEFAULT)
approach. See this commit. You can just check out the branch improve-config-new-api
and see the last lines of the playground. Run it and you will get the print statements in your console. You will also get nice IntelliSense with this one where I tried to expose as few internal information as possible to the public:
def _set_default_config() -> None: | ||
global configuration # pylint: disable=global-statement | ||
|
||
configuration = _default_config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you define a function for the one-line statement
configuration = _default_config
that you could just place like that at the end of the file?
if sigfigs == ChangeType.NO_CHANGE: | ||
sigfigs = configuration.sigfigs | ||
if sigfigs is None: | ||
sigfigs = _default_config.sigfigs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, haven't thought about doing it that way. Clever!
Whenever you copy-paste something, you might want to ask yourself if there's not a better way to achieve things. This will make the code look nicer and reduce maintability costs. Notice that you have the same if
going on for every single parameter. Please extract this to a separate method, e.g. set_config_parameter()
or alike.
@@ -1,3 +1,3 @@ | |||
from api.config import config_init, config | |||
from api.config import config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, that's a good change since this config_init
and config
was indeed a bit weird to use in the first place.
Please also make sure that the pipeline is completely green before requesting a review next time, thanks ;) |
Currently, there are two methods to change config:
config_init
andconfig
. This can be confusing for users. For this reason, with this PR, there will only beconfig()
. Each argument can be passed one of the following three value types:None
: If this is passed, the config parameter is reseted to default.ChangeValue.NO_CHANGE
: This custom type is used internally in order to not change parameters by default.This PR resolves #36.
Usage example