Skip to content
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

False positives in alembic revisions #1237

Closed
diego-pm opened this issue Feb 19, 2025 · 7 comments
Closed

False positives in alembic revisions #1237

diego-pm opened this issue Feb 19, 2025 · 7 comments
Labels
A-exclude Area: automatic and user-controlled exclusions C-bug Category: bug S-triage Status: New; needs maintainer attention.

Comments

@diego-pm
Copy link

diego-pm commented Feb 19, 2025

The linter gives a false positive in version numbers from alembic revision.

For example, the filename daa52d289898_description.py is corrected to data52d289898_description.py
and the revision numbers included in the file also give a false positive:

"""Description...

Revision ID: daa52d289898  # `daa` should be `data`
Revises: d1a839b16d12
Create Date: 2025-02-18 12:10:22.253105
"""
...
# revision identifiers, used by Alembic.
revision = "daa52d289898"  # `daa` should be `data`
down_revision = "d1a839b16d12"
branch_labels = None
depends_on = None
...
@diego-pm diego-pm added A-dict S-triage Status: New; needs maintainer attention. labels Feb 19, 2025
@epage
Copy link
Collaborator

epage commented Feb 19, 2025

Note that the template you filled out was specifically geared towards english words and does not make sense with other forms of identifiers.

It would also be good to define what an alembic revision as its not something I've ever come across.

If this is just another form of sha, we have #415 and #484.

@epage epage added A-exclude Area: automatic and user-controlled exclusions and removed A-dict labels Feb 19, 2025
@diego-pm
Copy link
Author

It seems that the issues you mentioned could also solve this, but this case has more information, so it might be easier to identify that the revision is a false positive, as the migration scripts follow a certain structure.

Alembic is a tool, used along sqlalchemy, to manage database schema migrations. Basically, an alembic revision is a python script that handles the upgrade/downgrade operations of a database schema migration.

The revision number or revision id is like a unique identifier of the database schema version. I think it is like an UUID or maybe some hash, not sure at all.

@epage
Copy link
Collaborator

epage commented Feb 19, 2025

this case has more information,

Could you define that format?

Alembic is a tool, used along sqlalchemy, to manage database schema migrations. Basically, an alembic revision is a python script that handles the upgrade/downgrade operations of a database schema migration.

The question I then have is whether this is something general enough to include built-in support for or if this should be left to user config via extend-ignore-identifiers-re or extend-ignore-re

@diego-pm
Copy link
Author

diego-pm commented Feb 20, 2025

The example code I show is the structure of alembic migration scripts. Also, the scripts always have 2 functions: upgrade and upgrade. And the filename always follows the 12 digit identifiers with some description.

Here is an example provided by the documentation https://alembic.sqlalchemy.org/en/latest/tutorial.html

"""Add a column

Revision ID: ae1027a6acf
Revises: 1975ea83b712
Create Date: 2011-11-08 12:37:36.714947

"""

# revision identifiers, used by Alembic.
revision = 'ae1027a6acf'
down_revision = '1975ea83b712'

from alembic import op
import sqlalchemy as sa

def upgrade():
    op.add_column('account', sa.Column('last_transaction_date', sa.DateTime))

def downgrade():
    op.drop_column('account', 'last_transaction_date')

But it is true that maybe this is too specific, and should be left to the user to exclude these patterns. The PR to detect hashes and UUIDs might be more general.

@epage
Copy link
Collaborator

epage commented Feb 20, 2025

The example code I show is the structure of alembic migration scripts. Also, the scripts always have 2 functions: upgrade and upgrade. And the filename always follows the 12 digit identifiers with some description.

I was less asking about the file format as we won't deal with that but the format of each of the non-words being treated as words.

For example, you mentioned the filename has a 12 digit identifier. Is it always data<hash>_*.py?

The PR to detect hashes and UUIDs might be more general.

We do have those already but we have at least #415. Our word splitting would not catch data<hash>. #484 could help.

@diego-pm
Copy link
Author

diego-pm commented Feb 20, 2025

The format of the non-words is the base 16 number (hash), there is no data (in general) in the non-word. The file name is generally <hash>_*.py.

@epage
Copy link
Collaborator

epage commented Feb 20, 2025

Sounds like this is then a duplicate of #415 and closing in favor of that.

@epage epage closed this as completed Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exclude Area: automatic and user-controlled exclusions C-bug Category: bug S-triage Status: New; needs maintainer attention.
Projects
None yet
Development

No branches or pull requests

2 participants