Remove explicit reference to xref partition and use declarative parti…#222
Remove explicit reference to xref partition and use declarative parti…#222
Conversation
…tion scheme by adding more constraints
Summary of ChangesHello @afg1, 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 streamlines the database's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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.
Code Review
This pull request updates the database query logic to use declarative partitioning for the xref table, instead of referencing a specific partition. The changes correctly replace the hardcoded partition table with the main xref table and add the necessary conditions (dbid and deleted) to the WHERE clause. My review includes a couple of suggestions to improve code style and maintainability, specifically regarding import ordering and the use of magic numbers.
| import psycopg2 | ||
| import psycopg2.extras | ||
| from bs4 import BeautifulSoup |
There was a problem hiding this comment.
According to PEP 8, imports should be sorted alphabetically within their groups. The previous order of these third-party imports was alphabetical. Please restore the alphabetical order to improve code readability and consistency.
| import psycopg2 | |
| import psycopg2.extras | |
| from bs4 import BeautifulSoup | |
| from bs4 import BeautifulSoup | |
| import psycopg2 | |
| import psycopg2.extras |
References
- PEP 8, the style guide for Python code, recommends that imports should be sorted alphabetically. Specifically, within each grouping (standard library, third-party, local), imports should be sorted. (link)
| & (xref.dbid == 11) # Added: was implicit in partition | ||
| & (xref.deleted == "N") # Added: was implicit in partition |
There was a problem hiding this comment.
These hardcoded values 11 and 'N' are magic numbers. To improve readability and maintainability, it's better to define them as named constants at the module level. For example:
# At the top of the file
RIBOVISION_DBID = 11
NOT_DELETED = 'N'Then you can use these constants in the query, making the code more self-documenting.
References
- Avoid using magic numbers. Define them as named constants to improve code readability and maintainability. This makes the code self-documenting and easier to update if the values change.
The database has for a long time had a partitioning scheme on the xref table based on inheritance and a big trigger function. This made adding new databases to RNAcentral more painful than it needed to be.
This PR accompanies structural changes in the database to enable declarative partitioning, and get rid of most of the manual modification of the trigger function that was previously required.
The only change in the code was one reference to a specific partiton, which we can get rid of by adding a couple more conditions to the query that were implicit by using the partition.
This whole change pending a test import into the test database, and testing website deployment from there before I synchronise the changes in the PRO database, hence draft PR