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

Template Yamls: Fix error due to colon in game name #4106

Merged

Conversation

gurglemurgle5
Copy link
Contributor

@gurglemurgle5 gurglemurgle5 commented Oct 26, 2024

What is this fixing or adding?

There's currently a bug where if a game has a colon in it's name, the outputted template yaml
has invalid syntax. This should fix that

How was this tested?

Generating template yamls, and looking at them to make sure they looked correct. I also generated a world with a couple games to make sure that yamls were still valid for generation.

NEEDED TESTING: apparently having colons in the game name might break the webhost? I haven't verified this for myself yet, but I'm gonna try and check it out

If this makes graphical changes, please attach screenshots.

there should be no changes for games that do not have colons in their names

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Oct 26, 2024
@Exempt-Medic Exempt-Medic added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. is: enhancement Issues requesting new features or pull requests implementing new features. affects: core Issues/PRs that touch core and may need additional validation. labels Oct 28, 2024
@Berserker66
Copy link
Member

that's not the proper fix, like further below, you'll want to use yaml dump.

thanks berserker66 for pointing out to me that I was doing this the
completly wrong way, so I fixed it up
@gurglemurgle5
Copy link
Contributor Author

Ok, just fixed it according to your suggestion

I haven't been able to actually test it yet with an apworld with a colon in the game name right now, but I can later. I have confirmed that this works for existing apworlds, though.

data/options.yaml Outdated Show resolved Hide resolved
@gurglemurgle5
Copy link
Contributor Author

So I added a function to clean up filenames, however, the regex it uses is really nasty. Should I replace it with something else, like looping over all chars and removing the illegal ones?

somehow i removed a space from the comment for the regex, so this adds
it back
this should fix the github actions issue maybe hopefully
@black-sliver
Copy link
Member

So I added a function to clean up filenames, however, the regex it uses is really nasty. Should I replace it with something else, like looping over all chars and removing the illegal ones?

We have Utils.get_file_safe_name for this

@gurglemurgle5
Copy link
Contributor Author

Ah, whoops, my bad, I should have checked for that. I've updated the code to use that function now.

@black-sliver
Copy link
Member

Was this properly tested now?

@gurglemurgle5
Copy link
Contributor Author

Was this properly tested now?

I have tested that it works with yamls, but I have not yet tested if it breaks things such as webhost

@black-sliver
Copy link
Member

I wrote a test for this: gurglemurgle5#1

Test: add test for option yaml with colon
@black-sliver black-sliver merged commit 6c9b7ec into ArchipelagoMW:main Nov 14, 2024
18 checks passed
@gurglemurgle5
Copy link
Contributor Author

ok currently game names with special characters break webhost, so this fix should not be relied on until webhost also gets fixed

@gurglemurgle5
Copy link
Contributor Author

it should be fine for apworlds, but not worlds that get added to core

black-sliver added a commit to black-sliver/Archipelago that referenced this pull request Nov 15, 2024
this mirrors the change in ArchipelagoMW/ArchipelagoMW#4106 in WebHost
black-sliver added a commit that referenced this pull request Nov 15, 2024
* WebHost: use new safe yaml template filename

this mirrors the change in ArchipelagoMW/#4106 in WebHost

* WebHost: install docs into safe filename and require docs to be named safe

* Test: update doc test for safe name

* WebHost: fix import order to not break ModuleUpdate
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: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. 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