Opened 10 years ago

Closed 10 years ago

#24275 closed Cleanup/optimization (wontfix)

Migration files should comply with PEP8

Reported by: Sambhav Satija Owned by: Sambhav Satija
Component: Migrations Version: dev
Severity: Normal Keywords: Migration, PEP8
Cc: f.ssat95@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The migration statements generated by the ​MigrationWriter should preferably be PEP8 compatible. This would ensure that when people run flake8 on their apps, they do not stumble unnecessarily due to warnings generated in the migration files.
In order to correct this, I've aimed to create a new class ​CleanWriter that MigrationWriter can inherit from, and can use its ​clean function before finally preparing the migration file. For lines which still can't be fixed, a # NOQA comment should be appended.
Currently there are a couple of bugs in the CleanWriter class, which if this ticket gets accepted, I'll fix and then create a PR.

Change History (9)

comment:1 by Tim Graham, 10 years ago

Triage Stage: Unreviewed → Accepted

I'd be interested to see this. In particular, the issue of long line lengths is annoying for me.

comment:2 by Sambhav Satija, 10 years ago

Owner: changed from f.ssat95@… to Sambhav Satija
Status: new → assigned

comment:3 by Sambhav Satija, 10 years ago

PR resides ​here. This is just a checkpoint for review and I will add unit tests after getting feedback.

comment:4 by Sambhav Satija, 10 years ago

Has patch: set

comment:5 by Markus Holtermann, 10 years ago

I don't really see the benefit. After all migration files are still mostly auto generated and are a usually excluded from the linter / style checker.

I don't like the additional complexity this PR adds. Especially since it could instead be handled with an --autopep8 option that has a dependency on "autopep8" as already mentioned by Aymeric on IRC.

comment:6 by Tim Graham, 10 years ago

Do you think we even need a flag? I think if autopep8 is installed, we could use it and just generate regularly if not. For anyone who wants the flag, I think it'd be annoying to have to specify it every time.

comment:8 by Tim Graham, 10 years ago

I have mixed feelings about this. It seems it would be cleaner if there were a more general hook so that projects could insert their own custom formatter (or add one that did autopep8), but in practice, I feel such a hook would hardly ever be used (and increase the barrier to using autopep8 so that most people wouldn't use it). Other opinions welcome.

comment:9 by Tim Graham, 10 years ago

Resolution: → wontfix
Status: assigned → closed

After some reflection, I think invoking autopep8 or another preferred linter from the command line is better than adding more complexity to Django.

Note: See TracTickets for help on using tickets.
Back to Top