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.

Sign up for GitHub

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

Jump to bottom

Add props to prevent typos in config properties #1795

Closed
wants to merge 10 commits into from
Closed

Add props to prevent typos in config properties #1795

wants to merge 10 commits into from

Conversation

sakurai-youhei
Copy link
Member

@sakurai-youhei sakurai-youhei commented Nov 2, 2023 •
edited
Loading

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 sakurai-youhei mentioned this pull request Nov 2, 2023
Open
@gbanasiak gbanasiak self-requested a review November 11, 2023 15:25
gbanasiak
gbanasiak reviewed Nov 11, 2023
View reviewed changes
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?

gbanasiak
gbanasiak reviewed Nov 21, 2023
View reviewed changes
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 •
edited
Loading

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

@sakurai-youhei @gbanasiak
Update esrally/props.py
fa242f2 
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.

@sakurai-youhei
Copy link
Member Author

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 #1798 was not a wrong decision. I appreciate the time and effort given to my PRs. Thank you very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gbanasiak gbanasiak gbanasiak left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DRY/Refactor hard coded config file keys
2 participants
@sakurai-youhei @gbanasiak