Skip to content

[Removal of attrs] Replacing with dataclass #3

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

Merged
merged 4 commits into from
Mar 23, 2025

Conversation

DEKHTIARJonathan
Copy link
Member

This PR removes attrs in favor of dataclass while changing an absolute minimum of things and remaining all the data validation and the unittests / fuzzy testing.

@DEKHTIARJonathan
Copy link
Member Author

DEKHTIARJonathan commented Mar 23, 2025

@mgorny can I merge this ? Or you prefer to keep attrs for now ?
We will have to switch to dataclass sooner or later anyway.

If you're good just approve the PR so that I can merge it

Copy link
Contributor

@mgorny mgorny left a comment

Choose a reason for hiding this comment

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

Sure. Sorry, I didn't realize you were waiting for my approval.

@DEKHTIARJonathan
Copy link
Member Author

@mgorny I'm trying to go for consensus and avoid taking decisions on my own

@DEKHTIARJonathan DEKHTIARJonathan merged commit 11648b9 into variant-json Mar 23, 2025
8 of 12 checks passed
@DEKHTIARJonathan DEKHTIARJonathan deleted the variant-json-dataclass branch March 23, 2025 15:03
DEKHTIARJonathan added a commit that referenced this pull request Mar 23, 2025
* [Transition to dataclass] `meta.py` file

* [Transition to dataclass] `config.py` file

* attrs removed

* Cleanup
DEKHTIARJonathan added a commit that referenced this pull request Mar 25, 2025
* Implement getting sorted variants aided by `variants.json`

Add a `variants_json` argument to `get_variant_hashes_by_priority()`
that enables the function to use variant definitions from
`variants.json` instead of evaluating all possible combinations.
This can yield significant speedup and lower resource use if only
a handful of variants need to be considered.

The function is implemented without any plugin API changes.  After all,
the current API should be simpler for plugin authors, and generating
separate variant lists should not pose a performance problem.

Implemention-wise, the function first converts all variant descriptions
from `variants.json` into `VariantDescription` classes.  Then, the list
of variants is fitlered to include only variants that match
configurations obtained from plugins.  Finally, they are sorted using
the configuration order.

I am not 100% sure about the sorting algorithm, but it seems to yield
exactly the same order at least for the `dummy-project` wheels.
The first element of the sort key is the number of metas in the variant,
to prioritize variants that matched more items.  It is followed by
priorities (indices in configuration order) of all metas, ordered
from the most preferred to the least preferred.

* Fix crash when a plugin is not installed

* Warn if providers or variant keys are missing

* Add a (failing) round-trip test for variants.json-based sorting

* Add CI workflow

* Fix variant sorting algorithm

* Add fuzzing tests (failing now)

* Include the known-failing example explicitly

* Fix sorting algorithm again

* Dependency Update

* Relax whitespace requirements in `VariantMeta.from_str()` (#4)

Allow any (or no) whitespace between `::` separators in
`VariantMeta.from_str()`, in order to make it more feasible for parsing
user input.  Unless I'm missing something, there should be no need
to strictly ensure that input is strictly formatted as `A :: B :: C`
(rather than, say, `A::B::C`), as long as we properly format output.

* [Removal of `attrs`] Replacing with `dataclass` (#3)

* [Transition to dataclass] `meta.py` file

* [Transition to dataclass] `config.py` file

* attrs removed

* Cleanup

* Abstract away plugin lookup and querying (#5)

* Remove duplicate plugin querying in get_variant_hashes_by_priority()

Refactor `get_variant_hashes_by_priority()` to reuse the list of plugin
names that is already included in the result of
`_query_variant_plugins()` rather than iterating over entry points
again.

* Abstract away plugin lookup and querying

Add a `PluginLoader` class that handles loading and querying plugins.
This removes some duplicate code, prepares for plugins having more
than one method in the future, and enables using a single cached plugin
list, rather than repeatedly querying and instantiating them.

* Replace `load_plugins()` with `create()` static method

* [Switch to Singleton]

---------

Co-authored-by: Jonathan DEKHTIAR <[email protected]>

* plugins: Rename `run()` to `get_supported_configs()`, add tests (#8)

Rename the `run()` API to `get_supported_configs()`, to prepare for
adding other plugin methods.  Fix bugs introduced while introducing
the singleton pattern.  Add tests.

---------

Co-authored-by: Jonathan Dekhtiar <[email protected]>
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.

2 participants