Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add props to prevent typos in config properties #1795

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

sakurai-youhei
Copy link
Member

@sakurai-youhei sakurai-youhei commented Nov 2, 2023

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.

Copy link
Contributor

@gbanasiak gbanasiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's DRY indeed and looks fine :) I left small comments below. Can you confirm we could convert to underscores (e.g. list.config.option -> list_config_option) just by changes in PropEnum and Property classes in the future?

esrally/props.py Outdated Show resolved Hide resolved
esrally/props.py Outdated Show resolved Hide resolved
esrally/props.py Outdated Show resolved Hide resolved
@sakurai-youhei
Copy link
Member Author

@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 #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?

esrally/props.py Outdated Show resolved Hide resolved
esrally/props.py Outdated Show resolved Hide resolved
@gbanasiak
Copy link
Contributor

gbanasiak commented Nov 21, 2023

@sakurai-youhei Thank you for iterating.

I think #1798 is a step in the right direction. It will help working on #1795 or similar, and tighten existing use of configuration, but by itself will not fully address #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 #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 #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.

Co-authored-by: Grzegorz Banasiak <[email protected]>
@sakurai-youhei
Copy link
Member Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DRY/Refactor hard coded config file keys
2 participants