-
Notifications
You must be signed in to change notification settings - Fork 27
Chore: Python-Spanner-Driver | Spannerlib Wrapper | Project Setup #656
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
Conversation
Summary of ChangesHello @bhatt4982, 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 lays the groundwork for the 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
This pull request does a great job of setting up the initial project structure for spannerlib-python, including configurations for building, packaging, testing, and linting. The setup is comprehensive. My review provides feedback to enhance the robustness and cross-platform compatibility of the build scripts, improve test configurations, and address minor issues in documentation and placeholder files. Key suggestions include adding set -e to the build script for safer execution, resolving the non-portable use of bash in noxfile.py, and correcting placeholder test files.
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/README.md
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/samples/requirements.txt
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/system/test_placeholder.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/tests/unit/test_placeholder.py
Outdated
Show resolved
Hide resolved
2035e5d to
319714a
Compare
|
/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 sets up the initial project structure for spannerlib-python. The setup includes configuration for building, testing, linting, and packaging using nox and setuptools. The overall structure is solid, but I've identified a few issues, primarily in the build scripts and CI configuration, that should be addressed. My main concerns are a critical path issue in the build script, lack of testing across all supported Python versions, and missing support for Intel-based Macs. I've left detailed comments with suggestions for each point.
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/README.md
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/noxfile.py
Outdated
Show resolved
Hide resolved
|
/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 sets up the initial project structure for spannerlib-python. The setup is comprehensive, including build scripts, packaging configuration with pyproject.toml, testing setup with nox, and CI-friendly shell scripts. My review focuses on improving the robustness and maintainability of the setup files. I've suggested a few changes to the README.md for clarity, improvements to the build script for simplicity and portability, and removal of some redundant code in noxfile.py and setup.py. Overall, this is a solid foundation for the new package.
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/setup.py
Outdated
Show resolved
Hide resolved
…kage installation after other dependencies.
…ript robustness with `set -e`.
…unit/test_placeholder.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…nt in build script
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
65e3a36 to
47ffbaa
Compare
olavloite
left a comment
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.
Feel free to address any review comments that need any changes in a follow-up PR.
...ib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/__init__.py
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/samples/requirements.txt
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/build-shared-lib.sh
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/noxfile.py
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/pyproject.toml
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/pyproject.toml
Outdated
Show resolved
Hide resolved
spannerlib/wrappers/spannerlib-python/spannerlib-python/pyproject.toml
Outdated
Show resolved
Hide resolved
…dle spannerlib library interaction (#657) * feat: Add initial Python wrapper for Spanner library, including internal message structures, error handling, and a protocol definition. * refactor: Introduce `_load_library` for library initialization, remove `_get_lib_path`, and update related tests. * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Pool class and relevant tests (#658) * feat: Introduce AbstractLibraryObject and Pool modules with their unit and system tests, replacing placeholder tests. * Chore: Python-Spanner-Driver - Added Connection class and relevant tests (#659) * feat: introduce Connection class and enable Pool to create and manage connections * Chore: Python-Spanner-Driver | Spannerlib Wrapper | Added Execute, ExecuteBatch and WriteMutation methods and relevant tests... (#660) * feat: Introduce `Rows` class and add `execute`, `execute_batch`, and `write_mutations` methods to `Connection`. * feat: Add SQL execution, batch DML, and mutation writing methods to Connection and expose Rows object. * Chore: Python-Spanner-Driver - Added Rows class, its methods and relevant tests... (#661) * test: add system tests for query, batch DML, and mutation operations, including emulator setup. * feat: Add `next()`, `metadata()`, `result_set_stats()`, and `update_count()` methods to the `Rows` class for fetching data and statistics, including unit tests. * test: Add system tests for `Rows` class covering metadata, stats, update count, and row iteration. * Chore: Python-Spanner-Driver - Added transaction functions in Connection class and relevant tests... (#662) * feat: Add transaction management with `begin_transaction`, `commit`, and `rollback` methods and their respective tests. * feat: add system and unit tests for transaction mutation writes, including commit and rollback scenarios. * feat: `update_count` returns -1 instead of 0 when row count stats are unavailable and updates the corresponding test. * refactor: remove direct Pool argument from Rows constructor and access via Connection. * feat: Add `IF NOT EXISTS` to `CREATE TABLE` and `IF EXISTS` to `DROP TABLE` statements, removing explicit exception handling. * feat: `write_mutations` now returns `None` for buffered mutations, updating its return type and related tests. * Update spannerlib/wrappers/spannerlib-python/spannerlib-python/google/cloud/spannerlib/internal/message.py Co-authored-by: Knut Olav Løite <koloite@gmail.com> * feat: Remove Python 3.8 fallback for `importlib.resources` and few docstring update * feat: Enhance SpannerLibError messages to include gRPC status names and add corresponding tests. * test: update SpannerLibError repr test message and code values --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Knut Olav Løite <koloite@gmail.com>
…and remove `importlib_resources` dependency.
…_DEPENDENCIES` and `mock`/`asyncmock` from `UNIT_TEST_STANDARD_DEPENDENCIES`.
Project setup for Spannerlib-Python