Sims 4: Rules Refactor Part 1 - #14
Conversation
This reverts commit 30a19ca.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds completed-aspiration event constants and a GOAL_TO_EVENT_MAPPING; updates Sims4World public API (new/changed create_* methods and event-location wiring during region setup); expands Items typing and skill entries; annotates location tables with explicit dict types and bumps version/metadata. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Player
participant Multiworld
participant Rules
participant Sims4World
participant RegionStore
Player->>Multiworld: start world setup
Multiworld->>Rules: set_rules(multiworld, player, options)
Rules->>Sims4World: create_regions()
Sims4World->>Sims4World: read AspirationGoal option
alt aspiration mapped via GOAL_TO_EVENT_MAPPING
Sims4World->>RegionStore: select target region
Sims4World->>Sims4World: create_event_location(event, region)
Sims4World->>Sims4World: create_event(event) -> event_item
Sims4World->>RegionStore: append event location and lock event_item
else no mapping
Sims4World->>RegionStore: build regions normally
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @worlds/sims4/__init__.py:
- Line 15: The import line imports an unused symbol; remove
set_completion_condition from the import and only import set_rules as
ts4_set_rules (i.e., change "from .Rules import set_completion_condition,
set_rules as ts4_set_rules" to "from .Rules import set_rules as ts4_set_rules"),
ensuring no other references to set_completion_condition exist in this module.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
worlds/sims4/Rules.pyworlds/sims4/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
worlds/sims4/__init__.py (1)
worlds/sims4/Rules.py (2)
set_completion_condition(83-140)set_rules(21-26)
🪛 GitHub Actions: Analyze modified files
worlds/sims4/__init__.py
[error] 11-11: F401 '.Items.skills_table' imported but unused
[error] 13-13: F401 '.Regions.sims4_skill_dependencies' imported but unused
[error] 13-13: F401 '.Regions.sims4_regions' imported but unused
[error] 15-15: F401 '.Rules.set_completion_condition' imported but unused
[error] 22-22: E302 expected 2 blank lines, found 1
[error] 31-31: E302 expected 2 blank lines, found 1
[warning] 66-66: E303 too many blank lines (2)
[error] 77-77: E501 line too long (123 > 120 characters)
[error] 119-119: E501 line too long (123 > 120 characters)
[error] 132-132: E501 line too long (134 > 120 characters)
[warning] 132-132: W505 doc line too long (134 > 120 characters)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.13 macos-latest
🔇 Additional comments (1)
worlds/sims4/__init__.py (1)
128-129: LGTM! Good refactoring to explicit parameters.The change from passing
selfto passing explicit parameters (self.multiworld,self.player,self.options) is a solid improvement. This makes the dependencies ofts4_set_rulesclearer and improves testability by decoupling from the world instance.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
worlds/sims4/Locations.py (1)
12-12: Add missing type annotation forparentparameter.The pipeline reports that the
parentparameter in the__init__method is missing a type annotation. Based on theBaseClasses.Locationparent class pattern and usage context, this should be typed asRegion.🔧 Proposed fix
- def __init__(self, player: int, name: str, address: Optional[int], parent): + def __init__(self, player: int, name: str, address: Optional[int], parent: Region):You'll also need to import
Regionat the top of the file:-from BaseClasses import Location +from BaseClasses import Location, Regionworlds/sims4/__init__.py (1)
128-131: Fix variable shadowing increate_regionmethod.The loop variable
location(a string from thelocationslist) is being reassigned to aSims4Locationobject on line 130. This variable shadowing causes the type checker errors reported in the pipeline. Use a different variable name for theSims4Locationinstance.🔧 Proposed fix
if locations: for location in locations: loc_id = self.location_name_to_id.get(location, None) - location = Sims4Location(self.player, location, loc_id, ret) - ret.locations.append(location) + loc = Sims4Location(self.player, location, loc_id, ret) + ret.locations.append(loc)
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
worlds/sims4/Locations.pyworlds/sims4/__init__.py
🧰 Additional context used
🪛 GitHub Actions: Analyze modified files
worlds/sims4/__init__.py
[error] 130-130: mypy: Incompatible types in assignment (expression has type "Sims4Location", variable has type "str").
[error] 131-131: mypy: Argument 1 to "append" of "list" has incompatible type "str"; expected "Location" [arg-type].
worlds/sims4/Locations.py
[error] 12-12: mypy: Function is missing a type annotation for one or more arguments [no-untyped-def].
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: fuzz
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
- GitHub Check: pyright
- GitHub Check: tests
- GitHub Check: mypy
- GitHub Check: flake8
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
🔇 Additional comments (2)
worlds/sims4/Locations.py (1)
28-28: Excellent type annotation additions!The explicit type annotations for
skill_locations_table,careers_locations_table,ptj_locations_table,aspiration_locations_table, andlocation_tableare exactly what was needed. These resolve the type inference issues mentioned in previous reviews and provide proper type safety throughout the codebase.Also applies to: 108-108, 573-573, 611-611, 887-887
worlds/sims4/__init__.py (1)
89-100: Well-structured helper methods.The new
create_item,create_event, andcreate_event_locationhelper methods are well-typed and logically sound. They properly separate concerns and make the code more maintainable. Thecreate_event_locationcorrectly creates locations without addresses (event locations) and ties them to regions.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @worlds/sims4/__init__.py:
- Around line 60-83: The line for AspirationGoal.option_neighborly_advisor in
GOAL_TO_EVENT_MAPPING is over 120 chars; break the tuple across multiple lines
to satisfy the line-length limit. Locate GOAL_TO_EVENT_MAPPING and replace the
single long entry for AspirationGoal.option_neighborly_advisor with a multi-line
form, e.g. put the key on its own line and split (EventNames.neighborly_advisor,
EventNames.neighborly_advisor_item) across two shorter indented lines so each
line stays under 120 chars while preserving the same mapping.
- Line 2: Update the import so Mapping is taken from collections.abc instead of
typing: replace the use of Mapping from the typing module with Mapping from
collections.abc (keep Any, ClassVar, Optional as needed from typing) to follow
PEP 585 / modern Python 3.9+ conventions; locate the import line that currently
references Mapping and change only that part.
- Around line 99-104: The create_event_location function inconsistently calls
Sims4Location with three args when region is None; update it so both branches
call Sims4Location(self.player, event, None, region) or explicitly pass None as
the fourth argument in the else branch (e.g., Sims4Location(self.player, event,
None, None)) so the parent/region parameter is always supplied; modify the else
branch in create_event_location to pass the missing fourth argument to
Sims4Location.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
worlds/sims4/Locations.pyworlds/sims4/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
worlds/sims4/__init__.py (4)
worlds/sims4/Locations.py (1)
Sims4Location(9-10)worlds/sims4/Items.py (1)
Sims4Item(22-23)worlds/sims4/Options.py (2)
AspirationGoal(8-29)Sims4Options(201-209)worlds/sims4/Rules.py (1)
set_rules(21-26)
🪛 GitHub Actions: Analyze modified files
worlds/sims4/__init__.py
[error] 79-79: E501 line too long (121 > 120 characters)
[error] 36-36: E302 expected 2 blank lines, found 1
[error] 2-2: E305 expected 2 blank lines after class or function definition, found 1
[error] 6-6: E501 line too long (160 > 120 characters)
[error] 1-1: E741 ambiguous variable name 'l'
🪛 Ruff (0.14.10)
worlds/sims4/__init__.py
2-2: Import from collections.abc instead: Mapping
Import from collections.abc
(UP035)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
🔇 Additional comments (9)
worlds/sims4/Locations.py (3)
84-84: LGTM: Corrected expansion reference for knitting skill.The expansion reference is now correctly set to
StuffNames.nifty_knitting, aligning with the proper naming convention used throughout the file.
98-99: LGTM: Formatting correction.The spacing for these skill location entries has been corrected for consistency.
104-104: Excellent: Type annotations added to all location tables.The explicit type annotations (
dict[int, Sims4LocationDict]) on all four location tables provide proper type safety and resolve the type inference issues mentioned in past reviews. This ensures that accessingdata["name"]and other dictionary keys will be correctly type-checked throughout the codebase.Also applies to: 569-569, 607-607, 883-883
worlds/sims4/__init__.py (6)
11-15: LGTM: Import cleanup.The unused imports (
skills_table,sims4_skill_dependencies,sims4_regions) have been removed, and new necessary imports (EventNames,AspirationGoal,Sims4Options) have been added. Theset_rulesimport no longer uses an alias, simplifying the code.
89-97: LGTM: Added return type annotations.The return type annotations (
-> Sims4Item) forcreate_itemandcreate_eventimprove type safety without changing functionality.
106-110: LGTM: Multi-line DLC set union.The DLC set computation has been reformatted across multiple lines for readability and to satisfy line length constraints. The logic remains unchanged.
Also applies to: 154-158
128-128: LGTM: Added type hints tocreate_regionparameters.The
locationsandexitsparameters now have proper type annotations (list[str] | None = None), improving type safety.
140-171: Excellent refactor: Event creation usingGOAL_TO_EVENT_MAPPING.The refactored
create_regionsmethod now uses theGOAL_TO_EVENT_MAPPINGdictionary to create event locations, replacing the previous if/elif chain. The logic correctly:
- Uses
goal_value(integer) for dictionary lookup- Creates the event location via
create_event_location- Appends it to the menu region
- Locks the corresponding item using
place_locked_itemThis approach is cleaner, more maintainable, and eliminates the type mismatch issues from earlier iterations.
Based on learnings, the PR author has tested this with Archipelago unit tests and fuzzer tools, but comprehensive world logic tests are tracked in issue #15.
175-175: LGTM: Simplifiedset_rulescall.The call now uses
set_rulesdirectly instead of thets4_set_rulesalias, simplifying the code.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @worlds/sims4/__init__.py:
- Line 2: The import line currently pulls Mapping from typing; update it to
import Mapping from collections.abc (per PEP 585) while leaving Any, ClassVar,
and Optional from typing unchanged so references to Mapping in this module
continue to work; replace the `Mapping` import in the top-level import statement
accordingly.
- Around line 99-104: The else branch of create_event_location is missing the
required parent/region argument for Sims4Location; always call Sims4Location
with four args per its __init__ signature (player, event, parent, region) —
e.g., keep the parent argument (None) and explicitly pass region as None when
region is None so both branches call Sims4Location(self.player, event, None,
region_or_None).
- Around line 60-83: One mapping entry in GOAL_TO_EVENT_MAPPING is over the
120-character limit; split the long line for
AspirationGoal.option_neighborly_advisor into multiple lines (e.g., put the
tuple elements on separate lines or wrap after the key) so the line length is
under 120 while preserving the tuple (EventNames.neighborly_advisor,
EventNames.neighborly_advisor_item) and existing indentation.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
worlds/sims4/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
worlds/sims4/__init__.py (3)
worlds/sims4/Locations.py (1)
Sims4Location(9-10)worlds/sims4/Items.py (1)
Sims4Item(22-23)worlds/sims4/Options.py (2)
AspirationGoal(8-29)Sims4Options(201-209)
🪛 Ruff (0.14.10)
worlds/sims4/__init__.py
2-2: Import from collections.abc instead: Mapping
Import from collections.abc
(UP035)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: fuzz
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.13 macos-latest
🔇 Additional comments (5)
worlds/sims4/__init__.py (5)
89-97: LGTM: Clear type annotations added.The return type annotations for
create_itemandcreate_eventimprove type safety and IDE support. The logic remains correct.
105-126: LGTM: Improved formatting for DLC set creation.The explicit set union for DLC values is now properly split across multiple lines, improving readability while maintaining the same logic.
128-138: LGTM: Type hints enhance clarity.The added type annotations for
locations,exits, and the return type make the method signature explicit and improve IDE support. The logic is unchanged and correct.
140-172: LGTM: Core refactor successfully implemented.The refactored
create_regionscorrectly uses the newGOAL_TO_EVENT_MAPPINGdictionary lookup. Key improvements:
goal_value(integer) is correctly used for the mapping lookup (line 165)aspiration_key(string) is correctly used for aspiration milestones (line 150)- Event location creation and item locking logic is clean and maintainable (lines 168-170)
- DLC filtering is properly formatted (lines 154-158)
This replaces the previous if/elif chain and eliminates the string-vs-integer comparison bug noted in earlier reviews.
174-175: LGTM: Simplified rules invocation.The direct call to
set_rulesis cleaner and consistent with the refactored imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @worlds/sims4/__init__.py:
- Line 156: The for-loop line "for aspiration in
sims4_aspiration_milestones[aspiration_key]: # (change this later, we'll need it
to do the multi aspiration thing that's in another branch)" violates E261 and
E501; fix by moving the trailing inline comment to its own line above the loop
or shortening it and ensuring there are two spaces before any remaining inline
'#' (e.g., add a standalone comment line like "# TODO: change this later for
multi-aspiration support" above the for loop), which both provides the required
two-space separation and prevents the line from exceeding the max length; update
the line using the symbols sims4_aspiration_milestones, aspiration_key, and
aspiration to locate the code.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
worlds/sims4/__init__.py
🧰 Additional context used
🧬 Code graph analysis (1)
worlds/sims4/__init__.py (5)
worlds/sims4/Locations.py (1)
Sims4Location(9-10)worlds/sims4/Items.py (1)
Sims4Item(22-23)worlds/sims4/Options.py (2)
AspirationGoal(8-29)Sims4Options(201-209)worlds/sims4/Rules.py (1)
set_rules(21-26)worlds/sims4/UT.py (2)
UTMixin(8-30)get_options_from_slot_data(19-30)
🪛 GitHub Actions: Analyze modified files
worlds/sims4/__init__.py
[error] 156-156: flake8: E261 at least two spaces before inline comment; E501 line too long. Also reported line continuation and other style issues in init.py.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.13 ubuntu-latest
- GitHub Check: Test Python 3.11.2 ubuntu-latest
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.11 windows-latest
- GitHub Check: Test hosting with 3.13 on ubuntu-latest
🔇 Additional comments (10)
worlds/sims4/__init__.py (10)
2-3: LGTM: Modern import style.The import of
Mappingfromcollections.abcfollows PEP 585 conventions for Python 3.9+.
11-16: LGTM: Clean import consolidation.Unused imports removed and new dependencies (
EventNames,AspirationGoal) properly added to support theGOAL_TO_EVENT_MAPPINGrefactor.
61-86: LGTM: Comprehensive goal-to-event mapping.All 18
AspirationGoaloptions are mapped. TheClassVarannotation ensures the mapping is shared across instances, and long entries are properly wrapped to stay within line limits.
92-97: LGTM: Return type annotation added.The explicit
-> Sims4Itemreturn type improves type safety and IDE support.
104-105: LGTM: Event item factory.Correctly creates a progression-classified event item with
Noneas the ID.
113-118: LGTM: Clean multiline formatting.The DLC set union is properly formatted across multiple lines for readability.
136-145: Good refactor to use helper method.The updated type hints (
list[str] | None) and use ofself.create_locationimprove clarity and consistency. Note: this inherits the potentialparentargument issue flagged increate_location.
167-172: Good mapping lookup pattern, but calls buggy path.The dictionary lookup with
get()andNonecheck is clean. However, line 170 callscreate_event_location(event_name)without passing aregion, which triggers the buggy else branch that's missing theparentargument. Once thecreate_event_locationfix is applied, this code will work correctly.
176-177: LGTM: Clean delegation to Rules module.The method correctly delegates rule-setting to the shared
set_rulesfunction.
99-102: The review comment is incorrect. TheLocation.__init__signature from BaseClasses.py shows that theparentparameter is optional with a default value ofNone:def __init__(self, player: int, name: str = '', address: Optional[int] = None, parent: Optional[Region] = None):The call
Sims4Location(self.player, name, location_id)is valid and will correctly defaultparenttoNone. There is noTypeError.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
worlds/sims4/Version.py (1)
29-29: Break long method signature to satisfy linter.The method signature exceeds 120 characters. Wrap the parameters across multiple lines.
🔧 Proposed fix
- def does_major_version_mismatch(client_version: tuple[int, int, int] | tuple[int, int, int, str], server_version: tuple[int, int, int] | tuple[int, int, int, str]) -> bool: + def does_major_version_mismatch( + client_version: tuple[int, int, int] | tuple[int, int, int, str], + server_version: tuple[int, int, int] | tuple[int, int, int, str] + ) -> bool:
🤖 Fix all issues with AI agents
In @worlds/sims4/__init__.py:
- Line 2: Remove the unused Optional import from the typing import line; update
the import statement that currently reads "from typing import Any, ClassVar,
Optional" to only include the actually used symbols (e.g., "Any, ClassVar") so
Optional is no longer imported and the linter warning is resolved.
📜 Review details
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
worlds/sims4/Version.pyworlds/sims4/__init__.pyworlds/sims4/archipelago.json
🧰 Additional context used
🧬 Code graph analysis (1)
worlds/sims4/__init__.py (5)
worlds/sims4/Locations.py (1)
Sims4Location(9-10)worlds/sims4/Items.py (1)
Sims4Item(22-23)worlds/sims4/Options.py (2)
AspirationGoal(8-29)Sims4Options(201-209)worlds/sims4/Rules.py (1)
set_rules(21-26)worlds/sims4/UT.py (2)
UTMixin(8-30)get_options_from_slot_data(19-30)
🪛 GitHub Actions: Analyze modified files
worlds/sims4/__init__.py
[error] 2-2: F401 'typing.Optional' imported but unused
worlds/sims4/Version.py
[error] 29-29: E501 line too long (176 > 120 characters)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test Python 3.12 ubuntu-latest
- GitHub Check: Test Python 3.13 windows-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 macos-latest
- GitHub Check: Test Python 3.13 windows-latest
🔇 Additional comments (7)
worlds/sims4/archipelago.json (1)
4-4: LGTM: Version bump is consistent with the refactor.The version increment from
2.0.0-beta2to2.0.0-beta3appropriately reflects the rules refactor changes in this PR.worlds/sims4/Version.py (1)
1-1: LGTM: Version constant updated consistently.The update from
rc2torc3aligns with the2.0.0-beta3version inarchipelago.json.worlds/sims4/__init__.py (5)
61-86: Excellent refactor: Dictionary mapping significantly improves maintainability.The
GOAL_TO_EVENT_MAPPINGsuccessfully replaces the previous if/elif chain with a clean, declarative mapping. This approach:
- Eliminates type comparison bugs (string vs. integer)
- Makes the goal-to-event relationship explicit and easy to verify
- Reduces code duplication
- Simplifies future additions
The mapping is comprehensive, correctly typed, and all 17 goal options are properly mapped to their corresponding event and item names.
92-108: LGTM: Helper methods follow clean, consistent patterns.The new helper methods provide excellent abstractions:
create_item: Properly looks up item metadata and classificationcreate_location: Correctly associates location with parent regioncreate_event: Creates progression item withNoneID (appropriate for events)create_event_location: Creates event location withNoneaddress and correct parentAll methods have clear signatures and handle the Sims4-specific object construction correctly.
110-131: LGTM: Improved readability with multiline DLC union and helper usage.The refactored
create_itemsmethod improves readability by:
- Breaking the DLC union across multiple lines (satisfies E501)
- Consistently using the
create_itemhelper- Maintaining the same logical flow
144-171: Excellent integration of the event mapping refactor.The refactored
create_regionsmethod successfully integrates theGOAL_TO_EVENT_MAPPING:Key improvements:
- Lines 164-169: Clean dictionary lookup replaces the error-prone if/elif chain
- Correctly uses
goal_value(integer) for the lookup, avoiding the previous string-vs-integer comparison bug- Properly creates the event location and locks the corresponding item
- Multiline DLC unions improve readability and satisfy linting
Logic flow:
- Retrieves the mapping for the selected goal (line 164)
- If a mapping exists, unpacks event and item names (line 166)
- Creates the event location (line 167)
- Locks the progression item to the location (line 169)
This is a significant maintainability improvement. The mapping approach makes the relationship between goals and events explicit and eliminates the previous bugs that were caught in earlier reviews.
173-174: LGTM: Clean delegation to rules module.The simplified call to
set_rulesproperly delegates to the Rules module with the required parameters.Acknowledgment of previous review work: The review history shows excellent collaboration between the author and reviewers. All previously identified issues have been addressed:
- ✅ Critical bugs fixed (wrong event names, type comparison errors, missing arguments)
- ✅ Code quality improvements (unused imports removed, line lengths fixed, type annotations added)
- ✅ Refactoring completed successfully (if/elif chain replaced with dictionary mapping)
This PR demonstrates strong attention to detail and responsiveness to feedback.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Katelyn Gigante <clockwork.singularity@gmail.com>
2 participants
What is this fixing or adding?
This branch is a refactor of the rules file to be more maintainable. Currently there are a few things left to be done, notably:
Migrate the career rules to use the has_skill helperMigration of career rules to the has_skill helper will happen in part 2 of the refactor.
How was this tested?
I haven't tested this yet. I'm still mid refactor, however testing will come soon.
Summary by CodeRabbit
New Features
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.