check in both locations for Packages.resolved and merge their contents if both are present #2
Conversation
…s if both are present
|
@CodiumAI-Agent /review |
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. |
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
![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:
- Ensure the merging logic for multiple Packages.resolved files handles duplicate packages gracefully, possibly prioritizing one location over another or merging version constraints intelligently.
- Reconsider the removal of the search path from error messages to aid in debugging when a version cannot be located.
- Evaluate the necessity and impact of logging search locations for resolved packages, potentially making it optional based on verbosity settings.
- Verify the thread safety of the new merging logic, especially if the plugin is expected to operate in a multi-threaded environment.
- Expand testing to cover edge cases, such as differing package versions between multiple Packages.resolved files and handling of empty resolved files.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Docstrings: all looks good
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| @@ -45,7 +45,7 @@ def check_for_updates(xcodeproj_path) | |||
| kind = requirement["kind"] | |||
|
|
|||
| if resolved_version.nil? | |||
| $stderr.puts("Unable to locate the current version for #{name} (#{repository_url}) in #{find_packages_resolved_file(xcodeproj_path)}") | |||
| $stderr.puts("Unable to locate the current version for #{name} (#{repository_url})") | |||
There was a problem hiding this comment.
suggestion (code_clarification): Consider including the search path in the error message.
Removing the path from the error message reduces the information available for debugging. Consider keeping it or providing an alternative way to identify where the search was conducted.
lib/spm_version_updates/plugin.rb
Outdated
| resolved_paths.each { |resolved_path| | ||
| resolved_versions.merge!( | ||
| JSON.load_file!(resolved_path)["pins"] | ||
| .to_h { |pin| | ||
| [pin["location"], pin["state"]["version"] || pin["state"]["revision"]] | ||
| } | ||
| ) | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Potential issue with merging versions from multiple Package.resolved files.
Merging versions from multiple Package.resolved files without handling duplicate keys could lead to unexpected behavior. Consider verifying that this approach aligns with the expected logic, especially in cases where the same package might be present in multiple resolved files with different versions.
| end | ||
| path = File.join(xcodeproj_path, "project.xcworkspace", "xcshareddata", "swiftpm", "Package.resolved") | ||
| locations << path if File.exist?(path) | ||
| $stderr.puts("Searching for resolved packages in: #{locations}") |
There was a problem hiding this comment.
suggestion (code_refinement): Logging search locations might not be necessary.
Consider if logging the search locations for Package.resolved files is necessary for the end user, or if it could be made optional or controlled by a verbosity/debug flag.
| @@ -189,6 +189,29 @@ module Danger | |||
| %r{Unable to locate the current version for kean/Nuke.*} | |||
| ).to_stderr | |||
| end | |||
|
|
|||
| it "Does report new versions for both possible Package.resolved locations" do | |||
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for when one location has a newer package version than the other.
This test case currently checks if new versions are reported when Package.resolved files are found in both locations. However, it would be beneficial to verify the behavior when one location contains a newer version of a package than the other. This could help ensure that the merging of contents from both locations is handled correctly.
spec/spm_version_updates_spec.rb
Outdated
| ) | ||
| end | ||
|
|
||
| it "Raised error when no Packages.resolved are present" do |
There was a problem hiding this comment.
nitpick (testing): Test description typo: 'Raised' should be 'Raises'.
spec/spm_version_updates_spec.rb
Outdated
| ) | ||
| end | ||
|
|
||
| it "Raised error when no Packages.resolved are present" do |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test case for when Packages.resolved files are present but empty.
This test case verifies the scenario where no Packages.resolved files are found. Adding a test case to handle the situation where Packages.resolved files exist but do not contain any package information (i.e., they are empty) could be beneficial. This would help ensure that the plugin behaves correctly in such scenarios, potentially raising an informative error or handling the situation gracefully.
No description provided.