Handle cases where package repo URLs might have different protocol (s… #3
Conversation
…sh or https) and/or .git suffix
|
@CodiumAI-Agent /review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3 +/- ##
==========================================
+ Coverage 87.87% 94.96% +7.08%
==========================================
Files 3 5 +2
Lines 132 139 +7
==========================================
+ Hits 116 132 +16
+ Misses 16 7 -9 ☔ View full report in Codecov by Sentry. |
PR Review
Code feedback:
✨ Review tool usage guide:Overview: With a configuration file, use the following template:
See the review usage page for a comprehensive guide on using this tool. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @hbmartin - I've reviewed your changes and they look great!
General suggestions:
- Consider implementing comprehensive validation for the
xcodeproj_pathto ensure it is not only non-nil but also valid and points to an existing file. - Refine the logging strategy to use a structured logging framework for better control and consistency across different environments.
- Ensure the
trim_repo_urlfunction robustly handles all expected URL formats, including edge cases, to prevent data processing issues. - Expand test coverage to include negative test cases for URL handling, ensuring the system gracefully handles malformed or unexpected URLs.
- Add test cases to verify the behavior when repositories are explicitly ignored, ensuring they are not processed even if new versions are available.
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
lib/spm_version_updates/plugin.rb
Outdated
| @@ -34,11 +34,14 @@ class DangerSpmVersionUpdates < Plugin | |||
| # The path to your Xcode project | |||
| # @return [void] | |||
| def check_for_updates(xcodeproj_path) | |||
| remote_packages = get_remote_package(xcodeproj_path) | |||
| raise(XcodeprojPathMustBeSet) if xcodeproj_path.nil? | |||
There was a problem hiding this comment.
suggestion (code_refinement): Consider validating xcodeproj_path for more than just nil.
It might be beneficial to also check if the path is not empty and points to an existing file to prevent runtime errors later.
lib/spm_version_updates/plugin.rb
Outdated
| resolved_versions = get_resolved_versions(xcodeproj_path) | ||
| $stderr.puts("Found resolved versions for #{resolved_versions.size} packages") | ||
|
|
||
| puts(resolved_versions) |
There was a problem hiding this comment.
suggestion (code_refinement): Consider using a more descriptive logging method.
Directly using puts might not be ideal for production code. It could be more appropriate to use a logging framework that can be configured for different environments.
lib/spm_version_updates/plugin.rb
Outdated
| remote_packages.each { |repository_url, requirement| | ||
| puts(repository_url) |
There was a problem hiding this comment.
suggestion (code_refinement): Ensure consistent logging levels and methods.
Similar to the previous comment, consider using a structured logging approach instead of puts for better control and consistency.
lib/spm_version_updates/plugin.rb
Outdated
| @@ -93,7 +86,7 @@ def get_resolved_versions(xcodeproj_path) | |||
| resolved_versions = resolved_paths.map { |resolved_path| | |||
| JSON.load_file!(resolved_path)["pins"] | |||
| .to_h { |pin| | |||
| [pin["location"], pin["state"]["version"] || pin["state"]["revision"]] | |||
| [trim_repo_url(pin["location"]), pin["state"]["version"] || pin["state"]["revision"]] | |||
There was a problem hiding this comment.
suggestion (code_refinement): Ensure trim_repo_url handles all URL formats correctly.
Given the method trim_repo_url is used to process repository URLs, it's crucial to ensure it correctly handles various URL formats, including those with or without protocols, and with different domain extensions.
lib/spm_version_updates/plugin.rb
Outdated
| # Removes protocol and trailing .git from a repo URL | ||
| # @param [String] repo_url | ||
| # The URL of the repository | ||
| # @return [String] | ||
| def trim_repo_url(repo_url) | ||
| repo_url.split("://").last.gsub(/\.git$/, "") |
There was a problem hiding this comment.
suggestion (code_refinement): Validate the trim_repo_url method's robustness.
It's important to ensure that trim_repo_url can handle edge cases, such as URLs that might not follow standard patterns, to avoid any potential data processing issues.
spec/spm_version_updates_spec.rb
Outdated
| @@ -212,6 +212,22 @@ module Danger | |||
| @my_plugin.check_for_updates("#{File.dirname(__FILE__)}/support/fixtures/NoPackagesResolved.xcodeproj") | |||
| }.to raise_error(CouldNotFindResolvedFile) | |||
| end | |||
|
|
|||
| it "Does report new versions with ssh and/or .git URLs" do | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding negative test cases for URL handling.
It's great to see a test case for positive scenarios where URLs with ssh and/or .git are handled correctly. However, to ensure robustness, consider adding negative test cases as well. For instance, test cases where the URL is malformed or does not follow the expected pattern. This will help ensure that the URL trimming and protocol handling are resilient to unexpected inputs.
spec/spm_version_updates_spec.rb
Outdated
| @@ -212,6 +212,22 @@ module Danger | |||
| @my_plugin.check_for_updates("#{File.dirname(__FILE__)}/support/fixtures/NoPackagesResolved.xcodeproj") | |||
| }.to raise_error(CouldNotFindResolvedFile) | |||
| end | |||
|
|
|||
| it "Does report new versions with ssh and/or .git URLs" do | |||
There was a problem hiding this comment.
suggestion (testing): Test case lacks verification for ignored repositories.
This test case effectively checks for the handling of URLs with different protocols and suffixes. However, it seems to miss the verification for scenarios where repositories are explicitly ignored (via ignore_repos). Adding a test case to ensure that ignored repositories are not processed, even if they have new versions available, would enhance the coverage and ensure the feature works as expected in all scenarios.
…sh or https) and/or .git suffix