Skip to content

Core: Allow common collections in OptionSet and OptionList constructors #2874

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

Conversation

Jouramie
Copy link
Contributor

@Jouramie Jouramie commented Feb 28, 2024

What is this fixing or adding?

Allow for usage of set, list, frozenset and tuple in .from_any(data) factory methods of OptionSet and OptionList.

I needed to add tuple support specifically to OptionSet, to use the .from_any(data) to parse the options, specifically the Mods combinations generated with itertools.combination().

Currently, if OptionSet.from_any(...) is called with a tuple, it is stringnified. For instance, that causes the tuple ("DeepWoods", "Tractor Mod") to be transformed into '("DeepWoods", "Tractor Mod")', which is then split on the , to create the OptionSet with the values '("DeepWoods"' and ' "Tractor Mod")', which are very obviously invalid. To add more to the issue, the validation is not even called because it fallbacks on the .from_text(...) factory method. This issue went unnoticed for weeks in our dev branch.

I could have changed the tuple to a set in each test, but it seems much simpler if the core supports it. Parsing and validation issues are solved at once.

One of the tests in question, FYI
    def test_given_mod_pairs_when_generate_then_basic_checks(self):
        if self.skip_long_tests:
            return
        for mod_pair in itertools.combinations(mod_data.all_mods, 2):
            world_options = {
                Mods: set(mod_pair)
            }
            with self.solo_world_sub_test(f"Mods: {mod_pair}", world_options, world_caching=False) as (multiworld, _):
                self.assert_basic_checks(multiworld)
                self.assert_stray_mod_items(mod_pair, multiworld)

Even if this technically adds an explicit dependency on typing-extensions, it was already a transitive dependency of Flask-Limiter, so it's not really a new dependency.

How was this tested?

Ran the tests, generated a plando seed with Stardew and Dark Souls III, plus generated a seed with all currently supported game (default options).

Honestly, those types are not even used with Generate.py, so I have big doubts it actually changes anything for generation in general, beside the insignificant performance drop from changing type(data) == list to isinstance.

If this makes graphical changes, please attach screenshots.

No graphical changes

@github-actions github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Feb 28, 2024
Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

Code looks good, idea seems reasonable

@beauxq
Copy link
Collaborator

beauxq commented Feb 29, 2024

@Jouramie Jouramie#1

@agilbert1412 agilbert1412 requested a review from beauxq March 1, 2024 21:39
Copy link
Collaborator

@beauxq beauxq left a comment

Choose a reason for hiding this comment

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

I looked over all of these changes with linting and type checking and thinking about the logic, and it looks good.

@agilbert1412 agilbert1412 added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Mar 1, 2024
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

Besides typing, basically only changes an isinstance() and adds a list() and those seem to be correct.seems to be correct.

@black-sliver black-sliver merged commit 37a871e into ArchipelagoMW:main Mar 3, 2024
@github-actions github-actions bot removed the waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. label Mar 3, 2024
@Jouramie Jouramie deleted the core/allow-more-iterables-in-option-parsing branch March 3, 2024 21:42
Comment on lines -849 to +851
def __init__(self, value: typing.List[typing.Any]):
self.value = deepcopy(value)
def __init__(self, value: typing.Iterable[str]):
self.value = list(deepcopy(value))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I should have noticed this. You changed the type var from Any to str, and I trusted it.

Copy link
Member

Choose a reason for hiding this comment

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

I missed it as well. We can at least partially blame the lack of unit tests :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I should have checked deeper how this was used. Definitely my bad on this one.

EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
…rs (ArchipelagoMW#2874)

* allow common collection in set and list option constructors

* allow any iterable of strings

* add return None

---------

Co-authored-by: beauxq <[email protected]>
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
…rs (ArchipelagoMW#2874)

* allow common collection in set and list option constructors

* allow any iterable of strings

* add return None

---------

Co-authored-by: beauxq <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects: core Issues/PRs that touch core and may need additional validation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants