-
-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(fix) handle case if examples are included above the working directory #67
Conversation
@coderabbitai review |
WalkthroughThe recent update introduces a nuanced adjustment specifically targeting the handling of absolute file paths within the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- lib/rspec_tracer/source_file.rb (1 hunks)
Additional comments: 1
lib/rspec_tracer/source_file.rb (1)
- 28-30: The comment on line 28 suggests that the method returns if the path is absolute, but the actual condition on line 29 checks if the file exists, regardless of whether it's an absolute path. This could lead to confusion. Consider clarifying the comment to reflect the actual check performed, which is whether the file exists at the given path.
Additionally, ensure that this logic correctly addresses the issue of handling examples located outside the working directory, as intended in the PR objectives.
end | ||
|
||
def file_path(file_name) | ||
# return if an absolute path, eg included examples above working directory | ||
return file_name if File.file?(file_name) | ||
# otherwise append root, to get an absolute path. | ||
File.expand_path(file_name.sub(%r{^/}, ''), RSpecTracer.root) | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [15-15]
The use of MD5 for generating file digests is noted. While MD5 is sufficient for non-security-related purposes, it's generally recommended to use more secure hashing functions like SHA256, especially to future-proof the code against potential vulnerabilities. Consider replacing MD5 with SHA256 for hashing file contents, which can be done with minimal changes:
- digest: Digest::MD5.hexdigest(File.read(file_path))
+ digest: Digest::SHA256.hexdigest(File.read(file_path))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai let's stick to changes in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@avmnu-sng, understood. I'll focus on the changes made in this PR. If there's anything specific within the PR's scope you'd like me to review or provide feedback on, please let me know.
Found an example when examples were included outside of the working dir pathing broke, and rspec-tracer would fail. Note below how file name is relative, for some examples, but absolute for the other example.
Any guidance or a tip on how to test would be great
Summary by CodeRabbit