Allow newer versions of arrow odbc and fix tests that return time field.#27
Allow newer versions of arrow odbc and fix tests that return time field.#27landwehrj wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello @landwehrj, 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 primarily focuses on improving dependency management and test reliability. It updates the Highlights
Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the arrow_odbc dependency to allow newer versions and fixes tests to accommodate changes in the returned data types. My review focuses on improving dependency management practices and test configuration. I've suggested specifying a minimum version for arrow_odbc instead of unpinning it completely to ensure future compatibility. I also recommend avoiding hardcoded credentials in the test connection string for better security and maintainability.
| xgt | ||
| pandas | ||
| arrow_odbc==0.2.0 | ||
| arrow_odbc |
There was a problem hiding this comment.
Unpinning a dependency completely can lead to unexpected build failures if a future version introduces breaking changes. It's better to specify a minimum version to ensure a baseline of compatibility while still allowing newer, non-breaking versions.
arrow_odbc>=0.2.0
|
|
||
| [options.extras_require] | ||
| odbc = arrow_odbc==0.2.0 | ||
| odbc = arrow_odbc |
There was a problem hiding this comment.
For better dependency management and to prevent potential issues for users installing this extra, it's advisable to specify a minimum version for arrow_odbc rather than leaving it unpinned. This ensures compatibility while allowing for updates.
odbc = arrow_odbc>=0.2.0
| @classmethod | ||
| def _setup_connector(cls): | ||
| connection_string = 'Driver={MariaDB};Server=127.0.0.1;Port=3306;Database=test;Uid=test;Pwd=foo;' | ||
| connection_string = 'Driver={MariaDB};Server=127.0.0.1;Port=3306;Database=test;Uid=test;Pwd=test;' |
There was a problem hiding this comment.
Hardcoding credentials, even for a local test database, is a security risk and reduces configuration flexibility. It's recommended to load sensitive information like passwords from environment variables. For example:
import os
password = os.getenv('MARIADB_TEST_PWD', 'test')
connection_string = f'Driver={{MariaDB}};Server=127.0.0.1;Port=3306;Database=test;Uid=test;Pwd={password};'There was a problem hiding this comment.
/gemini It's creating the docker image there during the test?
There was a problem hiding this comment.
No, the _setup_connector method itself doesn't create the Docker image. This method is responsible for defining the connection string to a MariaDB instance. Typically, in a test environment like this, the MariaDB Docker container would be spun up by a test fixture (often defined in conftest.py) before these tests run.
My suggestion to use an environment variable for the password (MARIADB_TEST_PWD) is still relevant. It allows you to configure the password for the test database (whether it's a Docker container or another setup) externally, making the test suite more flexible and secure. For example, if the Docker container's password changes, you only need to update the environment variable, not the code.
1 participant
No description provided.