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

Core: organize files on ingest via alpha, not ascii #3029

Merged

Conversation

kedNalatacId
Copy link
Contributor

Please format your title with what portion of the project this pull request is
targeting and what it's changing.

ex. "MyGame4: implement new game" or "Docs: add new guide for customizing MyGame3"

What is this fixing or adding?

Use alphabetic rather than asciibetic sorting when ingesting files for generation (upper and lower case letters will be interspersed rather than upper case before lower case. This doesn't fully order names. It's unclear if that would make things easier or harder (having all seeds together from a single file may make them easier to find).

How was this tested?

Generated many seeds.

If this makes graphical changes, please attach screenshots.

Screenshot 2024-03-24 at 23 04 57

@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 Mar 25, 2024
Generate.py Outdated
@@ -120,7 +120,7 @@ def main(args=None, callback=ERmain):
raise ValueError(f"File {fname} is invalid. Please fix your yaml.") from e

# sort dict for consistent results across platforms:
weights_cache = {key: value for key, value in sorted(weights_cache.items())}
weights_cache = {key: value for key, value in sorted(weights_cache.items(), key=lambda k: k[0].lower())}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea here that we're doing case-insensitive sorting only on the first letter, because it's unlikely to matter on the other letters?

If so, I'd rather do the whole string; it seems unlikely that sorting the filenames is a hot path (if it was, we probably couldn't afford to log all of them). The Python docs suggest key=str.casefold as a way to do case-insensitive sorting, which seems more self-explanatory to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sort is operating on tuples, such as the below:

('user.yaml',
 ({'game': {'accessibility': 'items',
                               'progression_balancing': 0},
   'game': 'game',
   'name': 'user'},))

The filename (what we're trying to sort) is the zeroeth portion of that tuple. I wasn't able to use str.lower() because it's not a simple string, so had to use a lambda expression instead. I have made the change from lower() to casefold().

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I could've sworn I checked that this wasn't a tuple and that's why the [0] was confusing me. Oh well.

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Tested that the changes work: they do. Can't really speak to whether or not there's a better way to do it though

@Exempt-Medic Exempt-Medic added is: enhancement Issues requesting new features or pull requests implementing new features. 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 Apr 5, 2024
@NewSoupVi NewSoupVi merged commit 3cc434c into ArchipelagoMW:main May 2, 2024
3 checks passed
jnschurig pushed a commit to Tranquilite0/Archipelago-SoulBlazer that referenced this pull request Jun 13, 2024
* organize files on ingest via alpha, not ascii

* Change from lower() to casefold()
qwint pushed a commit to qwint/Archipelago that referenced this pull request Jun 24, 2024
* organize files on ingest via alpha, not ascii

* Change from lower() to casefold()
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: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants