alteryx / alteryx/woodwork

Possible improvements to tracking unset config vars

Aperta
#1,054 0 commenti 1 reazione 0 assegnatari Vedi su GitHub
refactor
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.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.