Add props to prevent typos in config properties
This PR will eliminate numerous strings around config by typo-proof enums.
In short,
cfg.opts("benchmarks", "local.dataset.cache")
will be rewritten to
cfg.opts(*props.BENCKMARKS.LOCAL.DATASET.CACHE)
The following points are aimed in the implementation:
- Not require any changes in section and key names.
- Support auto-completion by IDEs.
- Keep the existing code width the same as possible.
- Prepare the way to describe config properties in the code.
- Enable to express both dot-notated sections and keys.
- Make unpreferable unpacking fail on properties in the middle of dot notation.
-> i.e. With
props.SECTION.DOT.NOTATED.KEY,func(*props.SECTION.DOT.NOTATED)raisesTypeError.
In return, the following restrictions are introduced:
- No way to declare the below kind of config keys.
-> i.e.[section] spam = many spam.eggs = a fewfunc(*props.SECTION.SPAM)raisesTypeError; you can still writefunc("section", "spam"), though. - Can't also express
camelCase,PascalCase,MACRO_CASE,kebab-case,COBOL-CASE, andbracket[notation]. - (possibly more)
Closes #1646
TODOs:
- [ ] Write unit tests.
- [ ] Write script to check typos between before and after strings elimination.
- [ ] Replace the rest of config
(section, key)pairs.
@gbanasiak Thank you for your comment.
However, I became a bit suspicious that this hacky code is sustainable, even though I opened this PR. Also, if you plan to convert dot notations (list.config.option) to underscores (list_config_option), there might be a more straightforward way. Another thought is about another PR https://github.com/elastic/rally/pull/1798, which is probably more acceptable for everyone.
What do you think? Can we archive (close without merging and leave) this PR until this enum constants gets really crucial?
@sakurai-youhei Thank you for iterating.
I think https://github.com/elastic/rally/pull/1798 is a step in the right direction. It will help working on https://github.com/elastic/rally/pull/1795 or similar, and tighten existing use of configuration, but by itself will not fully address https://github.com/elastic/rally/issues/1646 due to the following:
- any combination of section and key is still allowed
- verification is done only if
cfginstance is properly annotated withOptional[config.Config]or similar.
This wasn't stated explicitly but I've assumed we've embarked on the following journey:
- introduce literals and selective
mypyconfiguration to provide a full list of sections and keys to work on next - introduce
props(gradually) - once all literals replaced with
props, switchConfigmethods to acceptpropsonly instead of section/key strings.
I've already got accustomed to https://github.com/elastic/rally/pull/1795 logic and don't find it hacky, but if you see a more straightforward solution we can pursue this instead. We already looked at 2 proposals earlier in https://github.com/elastic/rally/issues/1646, and both were more repetitive in nature than this PR.
We can obviously also close this draft if you don't want to invest more time in it. Enum constants are not a top priority. It is a nice to have feature of the code. Also, it is understood community contributions are not "guaranteed". It's your free time after all.
The switch from dot notation to underscores is an option for the future, but I don't think we want to introduce such breaking change now. I was just confirming this proposal is flexible enough to accommodate this path, I think it is.
@gbanasiak Thank you for your comment.
[1]
once all literals replaced with
props, switchConfigmethods to acceptpropsonly instead of section/key strings.
The above didn't come up in my mind, but that's a good idea indeed.
[2]
The switch from dot notation to underscores is an option for the future, but I don't think we want to introduce such breaking change now. I was just confirming this proposal is flexible enough to accommodate this path, I think it is.
Thinking about it over a long period, the answer is YES. The props can be reworked when the time will have come. I thought the switch would happen in the near future. But if it's not, I don't have anything other than this PR to achieve the DRY as of now.
[3]
I've already got accustomed to #1795 logic and don't find it hacky
That's good to know. :) OK, I will resume working on this PR once #1798 gets done.
I'm closing this PR to clean up my PR list. Please feel free to reopen PR and/or ping me anytime.
P.S. Although @gbanasiak has welcomed this approach once, I've been questioning this PR by myself because of the hackiness. But I believe introducing mypy through https://github.com/elastic/rally/pull/1798 was not a wrong decision. I appreciate the time and effort given to my PRs. Thank you very much.