search

sqlalchemy / alembic

A database migrations tool for SQLAlchemy.
MIT License
2.89k stars 247 forks source link

D103 linter warnings from script.py.mako templates #1567

Open peterjc opened 2 weeks ago

peterjc commented 2 weeks ago

Describe the bug

Scripts created under <project>/alembic/versions/<name>.py from the Mako template fail the pydocstyle D103 style check, e.g. via flake8 or ruff check:

Expected behavior

The templates would include minimal docstrings for the upgrade and downgrade functions.

To Reproduce

Followed the tutorial, having enabled ruff check in alembic.ini and then run this with a trailing dot in the message to avoid D400 or D415.

$ alembic revision -m "Original schema."

Or without using this via the configuration, follow that with:

$ ruff check --select D103  */alembic/versions/*.py
xxx/alembic/versions/7c4c8748a5e0_original_schema.py:39:5: D103 Missing docstring in public function
   |
39 | def upgrade() -> None:
   |     ^^^^^^^ D103
40 |     pass
   |

xxx/alembic/versions/7c4c8748a5e0_original_schema.py:43:5: D103 Missing docstring in public function
   |
43 | def downgrade() -> None:
   |     ^^^^^^^^^ D103
44 |     pass
   |

Found 2 errors.

Error

# Copy error here. Please include the full stack trace.

Versions.

Additional context

PR to follow.

Have a nice day!

zzzeek commented 2 weeks ago

hi -

I disagree with these linter rules and I would disable them. you can change your own script.py.mako to resolve. feel free to send a pull request otherwise I would close this.

peterjc commented 2 weeks ago

I agree not everyone will use these rules, and they can be disabled on a file by file basis if the rest of the project does.

However, the PR is simple and small, and will mean less work for those alembic users which do run with the docstring linter rules enabled.

CaselIT commented 2 weeks ago

I'm personally -1 here. The suggested doc strings in the PR don't add anything in my point of view. As mike mentioned alembic init is a one time thing and you are supposed to customize your env.py and make template to fit your project.

Up to you mike, but I would close this and not merge the PR

zzzeek commented 2 weeks ago

I think it's harmless to have a basic docstring in the upgrade/downgrade method. I think a little bit of putting a more complete description in the docstring as to how the code gets there and that it should be edited etc. but then I dont konw that I want a huge paragraph being spit out there.