feat!: change AssetCache config structure (#186)#188
Conversation
📝 WalkthroughWalkthroughThis PR adds an optional ChangesCache directory override
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as server.py CLI
participant Loader as get_cached_config/generate_config
participant Config as obj_type
participant Cache as asset_cache
CLI->>Loader: cache_directory=args_ns.cache_directory
Loader->>Config: obj_type(data_dict, base_dir, cache_directory)
Config->>Config: merge cache_directory into game_setup
Config->>Cache: resolve dir_path from cache_directory or fallback
Cache-->>Config: asset_cache initialised
Related PRs: None identified. Suggested labels: enhancement, cache Suggested reviewers: Windows81 🐇 A cache directory, tucked in with care, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Source/game_config/__init__.py`:
- Line 134: The expression in the object-type loading path is a dead statement
with no effect, so update the relevant code around
obj_type.server_core.place_file to either assign its value where intended or
remove the line entirely if it is unnecessary. Verify the surrounding logic in
the config initialization flow so the ServerCore/ObjectType handling still
behaves correctly after the cleanup.
In `@Source/game_config/structure.py`:
- Line 37: The cache_directory option is being resolved through
wrappers.path_str, which currently anchors relative paths to the config/base
directory instead of the launch cwd. Update the Structure config handling for
cache_directory so CLI-provided relative paths are interpreted from the caller’s
working directory, using the relevant config parsing path in
Source/config_type/types/__init__.py and the cache_directory field in Structure
as the locating points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0fc96597-0ab5-408d-ba3d-c5e64543b050
📒 Files selected for processing (3)
Source/game_config/__init__.pySource/game_config/structure.pySource/launcher/subparsers/args_launch_mode/server.py
| 'server_core': {'place_file': {'rbxl_uri': rbxl_file}}, | ||
| 'game_setup': {'roblox_version': version.name}, | ||
| } | ||
| obj_type.server_core.place_file |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Dead statement.
obj_type.server_core.place_file has no effect. Confirm intent (a missing assignment?) or drop it.
♻️ Remove
}
- obj_type.server_core.place_file
base_dir = util.resource.retr_full_path(util.resource.dir_type.MISC)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj_type.server_core.place_file |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 134-134: Found useless expression. Either assign it to a variable or remove it.
(B018)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/game_config/__init__.py` at line 134, The expression in the
object-type loading path is a dead statement with no effect, so update the
relevant code around obj_type.server_core.place_file to either assign its value
where intended or remove the line entirely if it is unnecessary. Verify the
surrounding logic in the config initialization flow so the ServerCore/ObjectType
handling still behaves correctly after the cleanup.
Source: Linters/SAST tools
| sqlite_path: wrappers.path_str = '_.sqlite' | ||
| clear_on_start: bool = False | ||
|
|
||
| cache_directory: wrappers.path_str | None = None |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect path_str wrapper resolution logic
fd -t f 'wrappers' Source | head
rg -nP -C4 'class\s+path_str|def\s+__.*path' SourceRepository: Windows81/Roblox-Freedom-Distribution
Length of output: 802
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the config field and its call sites.
fd -t f 'structure.py' Source
fd -t f 'wrappers.py' Source/config_type/types
rg -n -C 4 'cache_directory|generate_config|get_cached_config|path_str\(' Source/game_config Source/config_type/typesRepository: Windows81/Roblox-Freedom-Distribution
Length of output: 7507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for docs/help around cache_directory or path semantics.
rg -n -C 3 'cache_directory|cache directory|cwd|current working directory|base_dir|relative path|path_str' README* SourceRepository: Windows81/Roblox-Freedom-Distribution
Length of output: 15231
Resolve --cache_directory from the launch cwd In Source/config_type/types/__init__.py:59-60, wrappers.path_str joins against base_dir, so a relative cache path is anchored to the config file directory (or MISC for generated configs), not the caller’s cwd. That makes --cache_directory ./foo land in different places depending on how the config is loaded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Source/game_config/structure.py` at line 37, The cache_directory option is
being resolved through wrappers.path_str, which currently anchors relative paths
to the config/base directory instead of the launch cwd. Update the Structure
config handling for cache_directory so CLI-provided relative paths are
interpreted from the caller’s working directory, using the relevant config
parsing path in Source/config_type/types/__init__.py and the cache_directory
field in Structure as the locating points.
|
im not reviewing what this ai bot bullshitted |
|
might have forgotten the readme |
|
Thanks for making these proposed changes! However, I don't want to make a drastic change to the way that game config works. What problem does this solve? Best. |
Windows81
changed the title
|
Hi, thanks. |
Windows81
changed the title
|
I hope this addresses your issue now: I want to avoid having a config option to be selected redundantly in more than one place. RFD's way of doing things is that all configuration options must only stray from their defaults if defined in |
2 participants
add suggestion #186