Core: Make excluded locations and priority locations excluded and remove unreachable code

[Exempt-Medic]

What is this fixing or adding?

If a location's progress_type was set to EXCLUDED and it was in a user's priority_locations, it would end up prioritized. In contrast, if something is in both exclude_locations and priority_locations, it ends up excluded. This makes it so that dev/world-excluded locations stay excluded and are removed from priority_locations (matching the behavior of using both excluded and priority through user-facing options) and a warning is logged if this overrides a user's priority_locations option.

This also simplifies part of an exception that could not be reached because of an earlier key verification exception with LocationSet key verfication.

How was this tested?

Generations with games that set progress_type and viewing spoiler logs.

[alwaysintreble]

i would much rather you remove the try except than pass on an exception which could have very bad implications

[Exempt-Medic]

i would much rather you remove the try except than pass on an exception which could have very bad implications

I'm not sure how you can do that, since removing the try/except causes get_location to throw a KeyError for locations that are in the datapackage but do not exist in that particular world. Could make it only pass on KeyError?

[ScipioWright]

Code looks fine, a player probably shouldn't get to priority a location if the world has excluded it so the change is good

[alwaysintreble]

i still don't like that this is ignoring exceptions. why are you removing handling of user error? world_excluded_locations also seems unnecessary. you can just remove directly from the priority locations set instead of creating another set just to use for removal.

[Exempt-Medic]

i still don't like that this is ignoring exceptions. why are you removing handling of user error? world_excluded_locations also seems unnecessary. you can just remove directly from the priority locations set instead of creating another set just to use for removal.

If you want to do that, feel free. It threw an error for me about modifying while iterating. This is also the same as what the error did earlier, so it's not making it worse

[Exempt-Medic]

i still don't like that this is ignoring exceptions. why are you removing handling of user error? world_excluded_locations also seems unnecessary. you can just remove directly from the priority locations set instead of creating another set just to use for removal.

And like I said earlier, this isn't removing user-error, this is handling that fact that locations that fail get_location but are still in the datapackage are supposed to be valid. If you have a better alternative, that maintains this behavior, feel free to PR it

[alwaysintreble]

And like I said earlier, this isn't removing user-error, this is handling that fact that locations that fail get_location but are still in the datapackage are supposed to be valid. If you have a better alternative, that maintains this behavior, feel free to PR it

According to the diff that's the old behavior which you're removing here?

[Exempt-Medic]

i still don't like that this is ignoring exceptions. why are you removing handling of user error? world_excluded_locations also seems unnecessary. you can just remove directly from the priority locations set instead of creating another set just to use for removal.

I keep thinking of new points, so sorry for the third reply, but can you give any example of a user error that does anything here? Every possible error is already covered, as explained in the PR's description. If a user includes a location that doesn't exist at all, it errors far earlier, and if a user includes a location that does exist but not for this particular instance of the game it shouldn't error, and so the continue happens instead.

[alwaysintreble]

Ah right, the options system validates the names way before this. It still feels wrong to have a try except where we just ignore the exception though. I wonder if it makes sense to at least log a warning?

[Exempt-Medic]

Ah right, the options system validates the names way before this. It still feels wrong to have a try except where we just ignore the exception though. I wonder if it makes sense to at least log a warning?

I could add logger.warning(f"Unable to prioritize location \"{location_name}\" in player {player}'s world because it does not exist.") but then excluded locations wouldn't have the same behavior

[ScipioWright]

I could add logger.warning(f"Unable to prioritize location \"{location_name}\" in player {player}'s world because it does not exist.") but then excluded locations wouldn't have the same behavior

If you did this, would it log an error for each location in a location group?

[Exempt-Medic]

I could add logger.warning(f"Unable to prioritize location \"{location_name}\" in player {player}'s world because it does not exist.") but then excluded locations wouldn't have the same behavior

If you did this, would it log an error for each location in a location group?

Yes, but it handling different from every other thing such as excluded locations, start hints, start location hints, etc. makes me not want to add it, at least not in this PR

[NewSoupVi]

I am perfectly fine with the try-except with continue and nothing else.