Swap min/max and warn on invalid ranges#25
Swap min/max and warn on invalid ranges#25eiron wants to merge 2 commits intosilasary:keymasters_keepfrom
Conversation
When a minimum option exceeded its corresponding maximum, the code previously forced the maximum to the minimum (losing the original maximum). This change swaps the min and max values instead, preserving the intended range order, and centralizes the warning text. It also prints the warning to stdout and logs it for lock_magic_keys, area_trials, and shop_items in KeymastersKeepWorld.
There was a problem hiding this comment.
Pull request overview
This PR adjusts Keymaster’s Keep world generation to handle invalid option ranges (min > max) by swapping the values instead of clamping, and emits a warning when such swaps occur.
Changes:
- Swap min/max for
lock_magic_keys,area_trials, andshop_itemswhen the range is inverted. - Update warning text to indicate a swap rather than clamping to the minimum.
- Add additional warning output paths (stdout + logging).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if self.lock_magic_keys_minimum > self.lock_magic_keys_maximum: | ||
| self.lock_magic_keys_maximum = self.lock_magic_keys_minimum | ||
| self.lock_magic_keys_minimum, self.lock_magic_keys_maximum = self.lock_magic_keys_maximum, self.lock_magic_keys_minimum |
There was a problem hiding this comment.
This swap assignment line exceeds the configured 120-character line length (Ruff/Black), which can fail lint. Consider wrapping the tuple assignment across multiple lines to stay within the limit.
worlds/keymasters_keep/world.py
Outdated
| warning_message = ( | ||
| f"Keymaster's Keep: {self.player_name} has a minimum lock magic keys value greater than the maximum. " | ||
| "Using minimum value for maximum." | ||
| "Swapping minimum and maximum values." | ||
| ) |
There was a problem hiding this comment.
The PR description mentions the warning text is centralized, but this message format is duplicated in each block. Either factor this into a small helper (e.g., a local function that formats/logs the swap warning) or update the PR description to match the implementation.
worlds/keymasters_keep/world.py
Outdated
|
|
||
| print(warning_message) | ||
| logging.warning(warning_message) |
There was a problem hiding this comment.
print(warning_message) will violate the repo’s Ruff config (T20 bans print) and may double-log since warnings already go to stdout via the logging stream handler. Remove the print and keep the logging.warning(...).
|
|
||
| if self.area_trials_minimum > self.area_trials_maximum: | ||
| self.area_trials_maximum = self.area_trials_minimum | ||
| self.area_trials_minimum, self.area_trials_maximum = self.area_trials_maximum, self.area_trials_minimum |
There was a problem hiding this comment.
This swap assignment line likely exceeds the configured 120-character line length (Ruff/Black) and may fail lint. Wrap the tuple assignment across multiple lines.
| self.area_trials_minimum, self.area_trials_maximum = self.area_trials_maximum, self.area_trials_minimum | |
| self.area_trials_minimum, self.area_trials_maximum = ( | |
| self.area_trials_maximum, | |
| self.area_trials_minimum, | |
| ) |
worlds/keymasters_keep/world.py
Outdated
|
|
||
| print(warning_message) | ||
| logging.warning(warning_message) |
There was a problem hiding this comment.
print(warning_message) will violate the repo’s Ruff config (T20 bans print) and may double-log since warnings already go to stdout via the logging stream handler. Remove the print and keep the logging.warning(...).
|
|
||
| if self.shop_items_minimum > self.shop_items_maximum: | ||
| self.shop_items_maximum = self.shop_items_minimum | ||
| self.shop_items_minimum, self.shop_items_maximum = self.shop_items_maximum, self.shop_items_minimum |
There was a problem hiding this comment.
This swap assignment line likely exceeds the configured 120-character line length (Ruff/Black) and may fail lint. Wrap the tuple assignment across multiple lines.
| self.shop_items_minimum, self.shop_items_maximum = self.shop_items_maximum, self.shop_items_minimum | |
| self.shop_items_minimum, self.shop_items_maximum = ( | |
| self.shop_items_maximum, | |
| self.shop_items_minimum, | |
| ) |
worlds/keymasters_keep/world.py
Outdated
|
|
||
| print(warning_message) | ||
| logging.warning(warning_message) |
There was a problem hiding this comment.
print(warning_message) will violate the repo’s Ruff config (T20 bans print) and also duplicates output because generation logging is already streamed to stdout via Utils.init_logging. Drop the print and rely on logging.warning(...) (or a dedicated logger) for user-visible warnings.
Ensure configuration minima/required values are properly swapped with their maxima/totals when inverted instead of overwriting them. Replace earlier assignment logic (which set total to required) with tuple swaps for artifacts and magic keys, and similarly for min/max ranges. Streamline warning emission by calling logging.warning directly with clearer messages describing the swap, and remove redundant print/log duplication.
|
I'd rather throw an Attempting to fix a typo by arbitrarily swapping values has the potential to inflict even more confusion than the current fix. |
When a minimum option exceeded its corresponding maximum, the code previously forced the maximum to the minimum (losing the original maximum). This change swaps the min and max values instead, preserving the intended range order, and centralizes the warning text. It also prints the warning to stdout and logs it for lock_magic_keys, area_trials, and shop_items in KeymastersKeepWorld.