Add local exercise repo fetching for testing local changes#40
Conversation
|
@VikramGoyal23 Is this ready for review? Would also be good to resolve copilot comments if they have been addressed as well to reduce cognitive load on reviewer |
|
@jovnc Yes, this is ready for another review |
|
@VikramGoyal23 This is more of an experimental change, so we will leave it here for now, and merge in during later weeks (or when E2E tests are ready) to prevent unnecessary regressions. |
- Typecheck `path` beofre use - Remove symlink copying - Clarify legacy fallthrough
…exercise-sources
VikramGoyal23
force-pushed
the
local-exercise-sources
branch
from
759cb9a to
dc79b4e
Compare
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
Also do include steps on how we can test this, and also relevant screenshots/videos to show it working, thanks!
app/configs/gitmastery_config.py
Outdated
| repository: str | ||
| branch: str | ||
| # "remote" or "local" | ||
| type: str = "remote" |
There was a problem hiding this comment.
Why is this the only non-optional field? If it doesn't exist, we can assume it to be remote, rather than set it to remote?
app/configs/gitmastery_config.py
Outdated
| path: Optional[str] = None | ||
|
|
||
| def to_url(self) -> str: | ||
| if self.type != "remote": |
There was a problem hiding this comment.
If doesn't exist, can assume it is remote if that's the default behaviour
app/configs/gitmastery_config.py
Outdated
| return cls(type="remote", username="git-mastery", repository="exercises", branch="main") | ||
| if isinstance(raw, dict): | ||
| typ = raw.get("type") | ||
| # explicit local |
There was a problem hiding this comment.
| # explicit local |
There was a problem hiding this comment.
Why do you wish to get rid of this comment?
There was a problem hiding this comment.
This is clear from the if statement itself, generally if the code is already self-explanatory, I don't think there's a need for comments.
| if raw is None: | ||
| return cls(type="remote", username="git-mastery", repository="exercises", branch="main") | ||
| if isinstance(raw, dict): | ||
| typ = raw.get("type") |
There was a problem hiding this comment.
| typ = raw.get("type") | |
| typ = raw.get("type", "remote") |
There was a problem hiding this comment.
This is already handled by the fallthrough.
app/configs/gitmastery_config.py
Outdated
| repository: Optional[str] = None | ||
| branch: Optional[str] = "main" | ||
| # local field | ||
| path: Optional[str] = None |
There was a problem hiding this comment.
ExercisesSource.path silently dropped during JSON serialization due to name collision with exclusion filter
The to_json() method at gitmastery_config.py:62-63 uses a default lambda that excludes any key named "path" or "cds" from all serialized objects, not just the top-level GitMasteryConfig. The new ExercisesSource.path field (line 22) stores the local exercises directory path, but its key name collides with this exclusion filter.
When config.write() is called (e.g., in on.py:130 or off.py:48), the nested ExercisesSource object's path field is silently excluded from the written JSON. On the next GitMasteryConfig.read(), from_raw receives a dict without a path key, creating an ExercisesSource(type="local", path=None). The subsequent ExercisesRepo.enter then raises ValueError("Path is required for using local exercises source"), breaking all exercise commands.
This was from Devin Review, please consider this.
There was a problem hiding this comment.
While this issue doesn't seem to arise in testing, I've made the change to prevent unexpected errors cropping up.
jovnc
left a comment
jovnc
left a comment
There was a problem hiding this comment.
I'm ok with this now, but would be good to show a demo that it works, or allow us to test that it works as expected.
Allows developers to set the exercise source as a local path, enabling fully local exercise testing. Backwards compatible and assumes lack of
typefield asremote.