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

Reorder imports to be more linter friendly #926

Closed
wants to merge 5 commits into from

Conversation

abitrolly
Copy link
Contributor

No description provided.

@zzzeek zzzeek self-requested a review September 18, 2021 14:51
Copy link
Member

@zzzeek zzzeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be applied consistently.

alembic/templates/generic/script.py.mako Show resolved Hide resolved
@abitrolly
Copy link
Contributor Author

Found 4 script.py.mako files. That should be it.

@abitrolly
Copy link
Contributor Author

Tested with python -m isort --check migrations/versions.

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I'm not convinced we need this, since in making isort happy, but probably will make other linters unhappy.

In any case, if we need to have it, there should not be a space between sqlalchemy and alembic, since both are considered as "packages" by isort

@abitrolly
Copy link
Contributor Author

abitrolly commented Sep 20, 2021

https://github.com/pypa/warehouse uses lines_between_types = 1 on their config, so my check worked with empty lines.

@CaselIT
Copy link
Member

CaselIT commented Sep 20, 2021

It again reinforces my point that formatting is something alembic cannot solve since it depends on the downstream users.

In any case the change is fine for me

@abitrolly
Copy link
Contributor Author

@CaselIT you're right, but at least I've tried to improve the contributor experience.

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 035b951 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Collaborator

New Gerrit review created for change 035b951: https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3093

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

hi -

propose abandon this change as it makes no sense. There is no standard "import order" and the proposed ordering fails the import ordering that the SQLAlchemy and Alembic projects themselves use. Please set up as follows:

setup.cfg

[flake8]
show-source = false
enable-extensions = G

exclude = .venv,.git,.tox,dist,doc,*egg,build
import-order-style = google

tox.ini

[tox]
isolated_build = True

[testenv:pep8]
basepython = python3
deps=
flake8
flake8-import-order
commands =
flake8 .

given two files with the two different imports here, the proposed one fails:

/mypackage/modelbase.py:2:1: I100 Import statements are in the wrong order. 'from alembic import op' should be before 'import sqlalchemy'

There is no fixed import order style in Python, and the post_process hooks are designed for this use case.

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3093

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

the existing ordering is more correct

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3093

@sqla-tester
Copy link
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/alembic/+/3093 has been abandoned. That means that at least for the moment I need to close this pull request. Sorry it didn't work out :(

@abitrolly
Copy link
Contributor Author

@zzzeek in the discussion #925 (reply in thread) you say it is OK, and during the review you say it makes no sense. What happened?

I don't really plan to invest more time into pushing this change. My experience shows that if any action meets a counter-action, which I can not explain, it is better to just abandon it. Open source is fun to people, because it allows to be opinionated, and if I don't like anything, I could just fork it.

Having said that I will repeat my opinion for the reference.

  1. If Alembic uses unusual formatting rules for own code, it should use the config rather than imposing these conflicting rules to projects that prefer the most popular no-config linting consensus.
  2. black became popular not because other linters didn't exist, were badly written or limited.

@CaselIT
Copy link
Member

CaselIT commented Sep 28, 2021

I honestly still don't understand what you want us to do..

If Alembic uses unusual formatting rules for own code, it should use the config rather than imposing these conflicting rules to projects that prefer the most popular no-config linting consensus.

why is it relevant how alembic own code is formatted? And what is the most popular linting consensus? where is it defined?
regarding no-config, alembic is requires it and is not able to work without a configuration, so I don't think that fits.

black became popular not because other linters didn't exist, were badly written or limited.

not sure how's that relevant. alembic is not trying to be a formatter..

@abitrolly
Copy link
Contributor Author

I honestly still don't understand what you want us to do..

It was an interesting discussion, that's it.

@abitrolly
Copy link
Contributor Author

why is it relevant how alembic own code is formatted?

Because it was one of the arguments to close this PR.

@abitrolly
Copy link
Contributor Author

And what is the most popular linting consensus? where is it defined?

It is not defined. It is measured. I explained one way to measure it in PyCQA/isort#1812 The UX discipline studies such things in a greater detail.

@abitrolly
Copy link
Contributor Author

not sure how's that relevant. alembic is not trying to be a formatter..

Yes, I should have expanded the thought to be complete. The popularity of the black is a proof that convenient defaults matter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants