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

Use OptionInfo type for option registration args/kwargs. #21766

Merged
merged 1 commit into from
Dec 17, 2024

Conversation

benjyw
Copy link
Contributor

@benjyw benjyw commented Dec 16, 2024

The options registration system has for a long time passed around
the naked args and kwargs used at registration time.

However the more recent implementation of class field-based
declarative options introduced an OptionsInfo dataclass to
encapsulate that data.

This PR spreads the use of OptionsInfo into the options
registration code. Advantages include:

  • More succinct code.
  • Proper type annotations.
  • No confusion about when to use * or **.

Note that this change renames OptionsInfo to OptionInfo,
since it represents the registration of a single option.
It also renames its fields to args and kwargs, since that
is the usage in most of the related code. Also, the name flag_options
was doubly confusing: A) A flag is just one aspect of an option, and
B) "options" is overloaded. This refers the knobs you can set when
registering an option, so using the word "options" for it is brain-hurting.
I considered "knobs", but figured "kwargs" was just as good, and already
in use.

@benjyw benjyw added the category:internal CI, fixes for not-yet-released features, etc. label Dec 16, 2024
@benjyw benjyw requested review from huonw, tdyas and thejcannon December 16, 2024 20:42
@benjyw
Copy link
Contributor Author

benjyw commented Dec 16, 2024

Review note: The diffs are pretty trivial, even though over 100 lines are touched.

Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

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

Makes sense, looks like a good improvement.

@benjyw
Copy link
Contributor Author

benjyw commented Dec 17, 2024

It should come as no surprise that this was also good prep work for my continued chipping away at the OptionsBootstrapper... :)

@benjyw benjyw merged commit e82a4ff into pantsbuild:main Dec 17, 2024
24 checks passed
@benjyw benjyw deleted the registrar_options_info branch December 17, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:internal CI, fixes for not-yet-released features, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants