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.

Sign up for GitHub

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: differentiate between unknown worlds and broken worlds in error message #2903

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Core: differentiate between unknown worlds and broken worlds in error message #2903

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion Generate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from worlds.alttp.Text import TextTable
from worlds.AutoWorld import AutoWorldRegister
from worlds.generic import PlandoConnection
from worlds import failed_world_loads


def mystery_argparse():
Expand Down Expand Up @@ -440,7 +441,11 @@ def roll_settings(weights: dict, plando_options: PlandoOptions = PlandoOptions.b

ret.game = get_choice("game", weights)
if ret.game not in AutoWorldRegister.world_types:
picks = Utils.get_fuzzy_results(ret.game, AutoWorldRegister.world_types, limit=1)[0]
picks = Utils.get_fuzzy_results(ret.game, list(AutoWorldRegister.world_types) + failed_world_loads, limit=1)[0]
if picks[0] in failed_world_loads:
raise Exception(f"No functional world found to handle game {ret.game}. "
f"Did you mean '{picks[0]}' ({picks[1]}% sure)? "
f"If so, it appears the world failed to initialize correctly.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to add a line along the lines of "If you're filing a bug report, the actual error you're looking for is further up"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have another PR that stops execution at that first error.

raise Exception(f"No world found to handle game {ret.game}. Did you mean '{picks[0]}' ({picks[1]}% sure)? "
f"Check your spelling or installation of that world.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like I'm nitpicking, but is it still worth mentioning the installation here, since the failed worlds are handled above?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is admittedly a hidden assumption here. At least currently, if you have failed worlds that means you manually installed one, since we ship with only ones that can initialize. So conversely you have to know what you did.

Copy link
Member

Choose a reason for hiding this comment

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

Idr exactly, but wasn't fuzzy matching case sensitive? If so, Factorio <-> factorio would be unable to match the 'F', which makes it less likely to find a good result.
Another idea to make this better would be to print "X worlds failed to load" in both code paths if failed_world_loads isn't empty.


Expand Down
5 changes: 5 additions & 0 deletions worlds/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,13 @@
"user_folder",
"GamesPackage",
"DataPackage",
"failed_world_loads",
}


failed_world_loads: List[str] = []


class GamesPackage(TypedDict, total=False):
item_name_groups: Dict[str, List[str]]
item_name_to_id: Dict[str, int]
Expand Down Expand Up @@ -87,6 +91,7 @@ def load(self) -> bool:
file_like.seek(0)
import logging
logging.exception(file_like.read())
failed_world_loads.append(os.path.basename(self.path).rsplit(".", 1)[0])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't really like this because world names may not fuzzy match well against their file path e.g. HK, kh2.

But I don't know a better alternative so the code looks good enough to me 🤷

Copy link
Member Author

@Berserker66 Berserker66 Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it's the best I could come up with without some much more complex string parsing to try and find the game name. Hopefully it incentivizes better package names.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can change this once we add metadata to the worlds, but that's not a thing yet.

return False


Expand Down
Loading