-
Notifications
You must be signed in to change notification settings - Fork 0
Setting default checksum file fix #45
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
base: setting_default_checksum
Are you sure you want to change the base?
Setting default checksum file fix #45
Conversation
Summary of ChangesHello @shubhangi-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the file upload mechanism within the Google Cloud Storage Ruby client library by introducing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
Thank you for this contribution. The changes to set a default checksum and fix checksum calculation for file paths are well-implemented and include relevant tests. My review primarily focuses on improving the long-term maintainability of the code by addressing several instances of code duplication that were introduced. I've provided specific suggestions to refactor this duplicated logic into shared methods and test modules.
|
/gemini review |
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.
Code Review
This pull request introduces a default checksum (:crc32c) for file creation, which is a good improvement for data integrity. The changes in bucket.rb and the associated test files are correct and properly test the new behavior. However, the refactoring of the checksum calculation logic in google/cloud/storage/file/verifier.rb has introduced a performance issue for file paths provided as strings and has made the code harder to understand. I've provided a detailed comment with a suggested replacement for the _digest_for method that is both more performant and easier to maintain.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>