Generate linter friendly output by default

[abitrolly]

Describe the bug

Generated migrations are not linter friendly. black fails, because migrations use single quotes. isort because imports are not properly ordered.

-from alembic import op
-import sqlalchemy as sa
+import sqlalchemy as sa
+
+from alembic import op

Perhaps there are more issues, but at least these two could already make it better.

Expected behavior

Generated migration to be black and isort compatible.

[CaselIT]

Hi,

you are free to modify the template file however you want.
Regarding the formatting, anyone has its own preference, so to address that alembic supports "post_write_hooks" for exactly this use case.

[abitrolly]

@zzzeek I am not using alembic, but the project on GitHub for which I submitted a PR is. I don't know why there is write_hooks feature. Maybe they considered that write hooks are insecure, or maybe they've started before this feature was implemented, or maybe the Pyramid framework considers running shell commands insecure. I don't know.

alembic requires configuration. it's right up in the tutorial. are you arguing that alembic's requirement that it be configured is unacceptable?

No, I am not arguing. You say that alembic requires configuration, but I don't see one in the Pyramid, so I just proposed to change the non-critical default output.

in particular, "reordering imports" has many different approaches

I doubt there is a linter that produces the output that alembic does. If there was one, I wouldn't touch this.

so what you ask is not actually even possible.

This is what I ask for #926

[zzzeek]

OK, your original issue was "Generated migrations are not linter friendly. " - the generated migrations are a product of a whole python rendering thing which necessarily needs to run through a tool like black to be nicely formatted.

if OTOH you are merely looking just for the two imports in the default template, that's a much narrower scope. but you mentioned "quotes" and things also. we rely on the formatting tools for that stuff.

your PR is fine (though it would have to include all the templates, not just that single one) but it doesn't address the whole problem you're referencing, if generated migrations are to work with linters then one needs to configure the post_write hooks.

I think your issue is still that "warehouse" is not configuring alembic completely. Why cant you take this issue up with them?

[abitrolly]

your PR is fine (though it would have to include all the templates, not just that single one) but it doesn't address the whole problem you're referencing, if generated migrations are to work with linters then one needs to configure the post_write hooks.

Yes, reordering imports doesn't fix the problem with single quotes, and as I don't know how the quotes are generated (manual AST walking or by a framework), my intention was just to fill an issue to see how far it can go. Neither I can test my changes right now.

[CaselIT]

@zzzeek poster just asked to replace single quote with double and reorder imports. Re-implementing black is not the goal to make the output friendly.

The original request was a lot broader, but in any case my reply holds, since even replacing single quotes with double is not immediate, considering also that python renders string with single quotes in repr.

If Alembic had a linter friendly output by default I would spent this hour doing something else. I could put a blame on PyPI maintainers for not putting the proper config in their repo, but I thought there might be a better way to fix the root cause.

Sorry but I think the issue lays with the pypi maintainers here. Alembic can be configured to respect a particular use formatting, so if a project requires one, it should be configured. There is "no root cause", since formatting is not an universal thing, and therefore cannot be fixed in an universal way. Alembic job is to render syntactically correct python, not to format it according the the particular formatting rule of a single project.

It took me about one hour to fix the formatting in pypi/warehouse#9972 because tests are not being run immediately. If Alembic had a linter friendly output by default I would spent this hour doing something else.

I think the solution to this would be to improve warehouse contributors guidelines so that contributors can follow a simple workflow. Again "a linter friendly output by default" does not exists, since the output depends on the linter used.

[CaselIT]

@CaselIT we've implemented this already :)

I don't think so. alembic init --help shows this:

usage: alembic init [-h] [-t TEMPLATE] [--package] directory

positional arguments:
  directory             location of scripts directory

optional arguments:
  -h, --help            show this help message and exit
  -t TEMPLATE, --template TEMPLATE
                        Setup template for use with 'init'
  --package             Write empty __init__.py files to the environment and version locations

[zzzeek]

hi there -

Alembic supports the feature you request fully. Please see: https://alembic.sqlalchemy.org/en/latest/autogenerate.html?highlight=black#applying-post-processing-and-python-code-formatters-to-generated-revisions which will illustrate how to plug in the "black" code formatter, or any other formatters you'd like.

[abitrolly]

This is needed by Pyramid here https://github.com/pypa/warehouse which uses Alembic, but I don't see any alembic.ini files there.

[zzzeek]

i would advise to take the issue up with warehouse that their alembic configuration story needs to improve.

[abitrolly]

pypi/warehouse#10053

[zzzeek]

there are many import ordering styles besides isort. We use zimports which will make use of any of the ordering styles used by flake8-import-order, and we use the "google" style. The imports as they are in the template right now are already compliant with "google" and changing them as proposed makes them non-compliant. these templates are generated into the user-project directory, which is not the "alembic" project; therefore both the "sqlalchemy" and "alembic" imports are third party imports, and should be together and alphabetically ordered.

as stated previously, there is no fixed import order in Python so people need to use the post_write_hooks to install the formatting they prefer.

[CaselIT]

we do follow that rule, see for example https://github.com/sqlalchemy/alembic/blob/master/alembic/operations/batch.py

[zzzeek]

I might have pointed this out earler. The "sqlalchemy" and "alembic" imports as stated inside of a migration template are both third party, because this is a Python file that renders in the user project where alembic is a third party dependency. that's why they are together in the current template.

[zzzeek]

note real world example in openstack:

https://github.com/openstack/neutron/blob/master/neutron/db/migration/alembic_migrations/dvr_init_opts.py#L18

[zzzeek]

their template: https://github.com/openstack/neutron/blob/master/neutron/db/migration/alembic_migrations/script.py.mako#L16

[abitrolly]

I might have pointed this out earler. The "sqlalchemy" and "alembic" imports as stated inside of a migration template are both third party,

Yes. I didn't understand that, the issue is closed. warehouse project should implement their own config to call two external tools via shell when generating migrations to format the migration output.