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

PEP8-format generated revisions #307

Closed
sqlalchemy-bot opened this issue Jul 14, 2015 · 14 comments

Comments

@sqlalchemy-bot
Copy link

commented Jul 14, 2015

Migrated issue, originally created by Adrian (@thiefmaster)

Even though they usually need to be modified a bit, it would be nice if the generated statements were PEP8-valid by default (besides maybe line-length..)

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 14, 2015

Michael Bayer (@zzzeek) wrote:

see also https://bitbucket.org/zzzeek/alembic/pull-request/11/changed-autogenerated-code-so-it-fits-80/diff. line length is very tough. E2 rules should be working as well as E11*, but for E12, E13, E3, E5, I'm not optimistic this is ever going to be "fixed". I know there are code formatters but those are going to just create more headaches.

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 14, 2015

Changes by Michael Bayer (@zzzeek):

  • removed labels: low priority
@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 14, 2015

Changes by Michael Bayer (@zzzeek):

  • added labels: autogenerate - rendering
@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

jmagnusson (@jmagnusson) wrote:

What about having one argument per line of a function call in the generated code? Should fix most problems of non PEP-8 compliant code. Example with what I mean:

op.alter_column(
        'blocki18n', 'links_to_text',
        existing_type=sa.TEXT(),
        server_default='',
        nullable=False
    )
@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

Adrian (@thiefmaster) wrote:

Exactly. The most common indentation in that case is this though:

op.alter_column(
    'blocki18n',
    'links_to_text',
    existing_type=sa.TEXT(),
    server_default='',
    nullable=False
)
@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Jul 25, 2015

jmagnusson (@jmagnusson) wrote:

Agreed. Just missed that ;-)

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Nov 23, 2016

Michael Bayer (@zzzeek) wrote:

Issue #393 was marked as a duplicate of this issue.

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Dec 5, 2016

ukrutt (@ukrutt) wrote:

UPDATE: oops, sorry, saw now that this code has changed in script.py.mako -- however when I tested this I had failed to update my own old mako file. Please disregard...

** please disregard comments below... **

It looks like a fair amount of work was done on this leading up to https://bitbucket.org/zzzeek/alembic/commits/tag/rel_0_8_9. I still get a few pep8 warnings though,

project/alembic/versions/175a74063cca_test.py:15:1: E402 module level import not at top of file
project/alembic/versions/175a74063cca_test.py:16:1: E402 module level import not at top of file

These are due to this code in the top of the generated file:

# revision identifiers, used by Alembic.
revision = '175a74063cca'
down_revision = '99c454c52997'
branch_labels = None
depends_on = None

from alembic import op
import sqlalchemy as sa

(specifically, the two imports.) It seems like this is a fairly straightforward thing to fix; however I'm new to this code in particular (and autogenerated code in general...)

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Apr 10, 2018

Wes Downey wrote:

Here is another example:

def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table('table_name',
    sa.Column('id', sa.Integer(), nullable=False),

This fails linting for these two rules:

  • E128 continuation line under-indented for visual indent
  • E122 continuation line missing indentation or outdented

The passing format would be like this:

def upgrade():
    # ### commands auto generated by Alembic - please adjust! ###
    op.create_table(
        'table_name',
        sa.Column('id', sa.Integer(), nullable=False),
@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Sep 2, 2018

Mike Waites (@mikeywaites) wrote:

Hey

I've recently started helping Mike out with sqlalchemy and alembic. On first inspection I agree with @ThiefMaster

op.alter_column(
    'blocki18n',
    'links_to_text',
    existing_type=sa.TEXT(),
    server_default='',
    nullable=False
)

Would solve the vast majority of peps -- That said there might be things I'm over looking. I'll get started on this on Tuesday and try and get a couple of "worst case scenario" tests cases together that highlight the issues. From there, the rendering of op instructions is fairly centralised so it should be fairly straight forward to prove out.

The work here will certainly be trying to produce test cases that produce all of the formatting issues. Perhaps it would make sense to deal with the really obvious ones first rather than going to deep.

It would be a great help if anyone interested in this fix could list any pep errors they have encountered not already listed above.

I'll report back here with my findings. 🤘

@sqlalchemy-bot

This comment has been minimized.

Copy link
Author

commented Sep 6, 2018

Neil Basu (@nbasu02) wrote:

@mikeywaites As a user what would also be a huge help to me is if we add a trailing comma. i.e.

op.alter_column(
    'blocki18n',
    'links_to_text',
    existing_type=sa.TEXT(),
    server_default='',
    nullable=False,
)
@sqla-tester

This comment has been minimized.

Copy link
Collaborator

commented Jan 18, 2019

Mike Waites has proposed a fix for this issue in the master branch:

Format autogenerated revisions https://gerrit.sqlalchemy.org/1060

@mikeywaites

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

Hey 👋

hoping there's still some people interested in this. I've been (very slowly) getting through some of these tickets and would love some feedback/testing on the proposed solution linked above.

The plan is to provide formatting via some simple hooks (with a couple of popular options baked in) to allow users to specify a third party library that alembic can defer formatting to. We think this is the best option as it means all the various nuances of pep8 can be catered for by libraries that will do a much better job than we will

I'm hoping to get this merged next week so any help before then would be greatly appreciated.

@m-aciek

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

@mikeywaites I will try to test this in the near future.

Is there chance for it to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.