rally icon indicating copy to clipboard operation
rally copied to clipboard

Add props to prevent typos in config properties

Open sakurai-youhei opened this issue 2 years ago • 3 comments

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:

  1. Not require any changes in section and key names.
  2. Support auto-completion by IDEs.
  3. Keep the existing code width the same as possible.
  4. Prepare the way to describe config properties in the code.
  5. Enable to express both dot-notated sections and keys.
  6. 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) raises TypeError.

In return, the following restrictions are introduced:

  • No way to declare the below kind of config keys.
    [section]
    spam = many
    spam.eggs = a few
    
    -> i.e. func(*props.SECTION.SPAM) raises TypeError; you can still write func("section", "spam"), though.
  • Can't also express camelCase, PascalCase, MACRO_CASE, kebab-case, COBOL-CASE, and bracket[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.

sakurai-youhei avatar Nov 02 '23 02:11 sakurai-youhei

@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 avatar Nov 16 '23 14:11 sakurai-youhei

@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 cfg instance is properly annotated with Optional[config.Config] or similar.

This wasn't stated explicitly but I've assumed we've embarked on the following journey:

  • introduce literals and selective mypy configuration to provide a full list of sections and keys to work on next
  • introduce props (gradually)
  • once all literals replaced with props, switch Config methods to accept props only 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 avatar Nov 21 '23 10:11 gbanasiak

@gbanasiak Thank you for your comment.

[1]

once all literals replaced with props, switch Config methods to accept props only 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.

sakurai-youhei avatar Nov 21 '23 13:11 sakurai-youhei

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.

sakurai-youhei avatar Oct 22 '24 11:10 sakurai-youhei