Conversation
SergeyTsimfer
left a comment
SergeyTsimfer
left a comment
There was a problem hiding this comment.
An overall impression is that not much has changed
Rewrite this thing from scratches with desired functionality (and tests) in mind
batchflow/config.py
Outdated
| pass | ||
| elif isinstance(config, Config): | ||
| self.config = config.config | ||
| super().__init__(config) |
There was a problem hiding this comment.
should not that start endless recursion?
There was a problem hiding this comment.
No, it simpy returns the given config
batchflow/config.py
Outdated
| raise TypeError(f'only str and Path keys are supported, "{str(key)}" is of {type(key)} type') | ||
|
|
||
| if isinstance(key, str): | ||
| key = '/'.join(filter(None, key.split('/'))) |
There was a problem hiding this comment.
If the goal is to deduplicate slashes, then .replace('//', '/') is much easier to read; also, if clause is not needed
There was a problem hiding this comment.
Agree, but what if we have three or more consecutive slashes? Also, I think that if clause is needed cause key could be another object, not necessarily a string
batchflow/config.py
Outdated
| if key in self: | ||
| self.pop(key) |
There was a problem hiding this comment.
I want here syntax like in the dict :
| if key in self: | |
| self.pop(key) | |
| _ = self.pop(key, None) |
There was a problem hiding this comment.
Also, why we can't write the put method in the way when we needn't pop before put?
There was a problem hiding this comment.
Also, why we can't write the
putmethod in the way when we needn'tpopbeforeput?
I think, we can't, because we want to update value if key is already in config. However, there are some cases when pop is necessary.
For example: config = Config({'a/b': 1, 'a/c': 2})
When we want to set a new value for key='a', config['a'] = dict(b=0, d=3), we want to get
config = {'a': {'b': 0, 'd': 3}}.
batchflow/config.py
Outdated
| if parent in config and isinstance(config[parent], Config): # for example, we put value=3 with key='a/c' into the | ||
| config[parent].update(Config({child: value})) # config = {'a': {'b': 1}} and want to receive {'a': {'b': 1, 'c': 3}} | ||
| else: | ||
| config[parent] = Config({child: value}) |
There was a problem hiding this comment.
We will silently rewrite items like {'a': 3} by {'a/b': 2}. Should it be a warning or exception?
There was a problem hiding this comment.
We will silently rewrite items like
{'a': 3}by{'a/b': 2}. Should it be a warning or exception?
I think, yes, a warning would be good here
batchflow/config.py
Outdated
| def __getattr__(self, key): | ||
| if key in self: | ||
| value = self.get(key) | ||
| value = Config(value) if isinstance(value, dict) else value | ||
| return value | ||
| raise AttributeError(key) | ||
|
|
There was a problem hiding this comment.
Maybe AttributeError should be defined in the self.get?
There was a problem hiding this comment.
Maybe
AttributeErrorshould be defined in theself.get?
But in this case we will catch AttributeError every time we want to retrieve nonexisting key from the dict, not only by .
| def __eq__(self, other): | ||
| self_ = self.flatten() if isinstance(self, Config) else self | ||
| other_ = Config(other).flatten() if isinstance(other, dict) else other | ||
| return self_.__eq__(other_) |
There was a problem hiding this comment.
What if other is a Config and not flatten?
There was a problem hiding this comment.
What if other is a
Configand notflatten?
He will be flattened anyway because if other is an instance of Config, isinstance(other, dict) is True
SergeyTsimfer
left a comment
SergeyTsimfer
left a comment
There was a problem hiding this comment.
Still overly complicated and keeps old code. Also, make sure that tests are working
EvgeniyS99
force-pushed
the
change_config
branch
from
7695c42 to
03b36cc
Compare
SergeyTsimfer
left a comment
SergeyTsimfer
left a comment
There was a problem hiding this comment.
Looks a lot better!
A few comments:
- there are some dev comments in the code, but not enough of them in the
_get/_putmethods - check that documentation is up-to-date
- add descriptive messages into raised errors: that would help greatly when keys are missing on various levels of nestedness
| return other << self | ||
|
|
||
| def __getstate__(self): | ||
| """ Must be explicitly defined for pickling to work. """ |
4 participants
A new version of config supports any hashable object as key (like a simple dict):
However, due to the fact that tests were written for the old version of config which supports only
stras a key, some of them are failed