Remove the resolved TODO from Sources::Local - #3146
Merged
Merged
Conversation
`Sources::Local#initialize` carries a TODO asking to resolve a relative source path from the directory of `rbs_collection.yaml`. That was done three and a half years ago and the comment was left behind. The TODO arrived in 61dd0e4, a force-push of #854 dated 2022-02-11, where the constructor was: def initialize(source_entry) # TODO: resolve relative path from dir of rbs_collection.yaml @path = Pathname(source_entry['path']) end There the path really was taken as written and resolved against the working directory, so the TODO described the code accurately. 86f4ec5, pushed to the same PR on 2023-02-08, added the `base_directory:` keyword to `Sources.from_config_entry` and changed the body to `base_directory / path`. `Config#sources` passes `@config_path.dirname` and `Lockfile.from_lockfile` passes `lockfile_path.dirname`, so from that point a relative path has been resolved from the directory holding the configuration -- exactly what the TODO asked for. Only the code was updated; the comment stayed. #854 ran from 2021-12-23 to 2023-02-17 and its commits were squashed into 718997e, so the TODO and the change that answered it landed in one commit and the leftover was invisible in the history. Close #3124 #854 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HSq3kKMB8ULhZrnL5urD7y
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sources::Local#initializecarries a TODO asking to resolve a relative source path from the directory ofrbs_collection.yaml. That was done three and a half years ago and the comment was left behind. This deletes it.Where the TODO came from
The comment arrived in 61dd0e4, a force-push of #854 dated 2022-02-11, where the constructor looked like this:
The path really was taken as written and resolved against the working directory, so the TODO described the code accurately at the time.
Where it was answered
86f4ec5, pushed to the same PR on 2023-02-08, added the
base_directory:keyword toSources.from_config_entryand changed the body tobase_directory / path:Config#sourcespasses@config_path.dirnameandLockfile.from_lockfilepasseslockfile_path.dirname, so from that commit on a relative path has been resolved from the directory holding the configuration, which is what the TODO asked for. Only the code was updated; the comment stayed.#854 ran from 2021-12-23 to 2023-02-17 and its commits were squashed into 718997e, so the TODO and the change that answered it landed in the same commit. That is why the leftover is not visible from
git log -Lon the file.Alternatives considered
#3124 removes the same comment, but also changes
@full_pathto(base_directory / path).expand_path. That is a separate change with its own behaviour: it only matters whenbase_directoryis relative, which therbsCLI never produces, sinceconfig_pathis either an absolute path fromConfig.find_config_pathor the expanded argument of--collection. It can be reached by callers that build aLockfilewith a relative path, such as Steep. Keeping the two apart lets this one land as the pure comment deletion it is, and leaves theexpand_pathquestion to be decided on its own merits with a test.Close #3124
#854
🤖 Generated with Claude Code
https://claude.ai/code/session_01HSq3kKMB8ULhZrnL5urD7y