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

Utils: Add a flatten function to Utils.py #3167

Closed
wants to merge 1 commit into from

Conversation

nex3
Copy link
Contributor

@nex3 nex3 commented Apr 17, 2024

What is this fixing or adding?

This function is useful for #3128 and may be useful for other
multiworlds. SM uses it as well, but only for the varia randomizer
that's pulled in from elsewhere so it doesn't make sense to re-use it
there.

How was this tested?

This uses essentially the same implementation as the function already
in the SM varia randomizer.

@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 Apr 17, 2024
@nex3 nex3 mentioned this pull request Apr 17, 2024
@Berserker66
Copy link
Member

Isn't this case already covered by the standard lib?
https://docs.python.org/3/library/itertools.html#itertools.chain

@beauxq
Copy link
Collaborator

beauxq commented Apr 17, 2024

A difference from itertools.chain is that this would flatten an arbitrary number of levels of lists.
But I'm not sure that's a good thing to support.

If a developer doesn't know statically how many levels deep their data structure is, that doesn't seem like a good pattern.
I would suggest that it should be refactored so that there is a known number of levels, so that itertools.chain or something more specific can be used.

Notice that the type annotations are not correct for arbitrary numbers of levels. It's not even clear to me how many levels it would have to be to get it right.

@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Apr 19, 2024
@nex3
Copy link
Contributor Author

nex3 commented Apr 23, 2024

There are two usability benefits here versus itertools.chain() that make this particularly well-suited for writing large chunks of data by hand (as you do when defining information for Archipelago games):

  1. It returns a list, rather than an iterator, so it can be more easily used without having to do additional generator syntax on top.

  2. It supports mixed values and iterables, so you can have a list that occasionally has sublists. I use this in my DS3 world updates to more easily generate "bundles" of items with different counts.

The fact that it supports arbitrarily nested iterables isn't important to me, so if that's an issue I can update it to support only one level of iterables as well as direct values.

@beauxq
Copy link
Collaborator

beauxq commented Apr 23, 2024

I think this would be a better to do this: nex3#13

Even if you don't like that, I still think this is too specific for Utils.py

@nex3 nex3 closed this Jun 10, 2024
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. is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants