Possible improvements to tracking unset config vars
- Lingua principale
- Python
- Stelle
- 155
- Fork
- 24
- Metriche di merge delle PR
- Nessuna PR unita negli ultimi 30g
Descrizione
Sort of a nitpick I noticed. But having a bit of discussion about it might be useful for other issues that are conceptually related.
With Python, the convention is often to use `None` to signify missing values. Here, we're using `-1` which seems like more of a C idiom: https://github.com/alteryx/woodwork/blob/0c3978f1d2ce1e1c6433421ca172330277a5f1a0/woodwork/config.py#L3
Also, we're checking specifically for this sentinel value pretty deep in some program logic: https://github.com/alteryx/woodwork/blob/0c3978f1d2ce1e1c6433421ca172330277a5f1a0/woodwork/type_sys/inference_functions.py#L110-L111
Someone looking at that function might wonder, "What's this value we're checking for? What's that about?" without realizing that it's a sentinel value defined in `woodwork.config`. We could have a module level constant that defines the value and that would help a little, but then people would have to be aware that they need to import that value and check for it. New contributors might not realize that they need to do that.
For optional config vars like the one above, maybe we could have some kind of specialized sentinel value that works in the manner shown below:
```python
class NotSet:
pass
NOT_SET = NotSet()
CONFIG_DEFAULT = {
...
"numeric_categorical_threshold": NOT_SET,
...
}
class Config:
...
def get_option(self, key):
...
if self._data[key] is NOT_SET:
# This can also be a specialized exception subclass. Doesn't have to be a built-in one.
raise ValueError(f"Config option is valid but not configured: {key}")
...
# Then, for code that needs to do something if that config var is set:
try:
threshold = ww.config.get_option("numeric_categorical_threshold")
except ValueError:
pass
else:
# Do whatever you want with the config var...
...
```
I kind of prefer something like this because people can just do the same try/except thing whenever they need to fetch an optional config var. And they don't need to know any details about how unset config vars are tracked internally.
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.