Skip to content

Conversation

lukeyeager
Copy link
Contributor

Close #3

This is a follow-up to #575. This is that terrifying diff I was talking about.

To reduce the terror, I've split this up into several commits:

  1. Fix most lint automatically with autopep8 (as I did with Fix remaining Python lint NVIDIA/DIGITS#1207)
  2. Manually fix all the lint with an obvious fix
  3. Manually fix lint in the docstrings that could potentially break the auto-generated documentation
    • I think I got it right, but I'd like for someone who understand the documentation generation tool to have a look

@lukeyeager lukeyeager changed the title Fix all pep8 lint [DON'T MERGE] Fix all pep8 lint Jan 27, 2017
@lukeyeager
Copy link
Contributor Author

I'd like to wait for #614 before merging this.

@apaszke
Copy link
Contributor

apaszke commented Jan 27, 2017

Can you rebase the PR to have the new travis config?

@desimone
Copy link
Contributor

desimone commented Jan 27, 2017

Big fan of pep8'ing things. The less mental space assigned to formatting -- whatever defaults are chosen -- the better in my opinion.

That said, since this is probably as good of a time as any to ask, why 120 vs 79 for column limit? I personally go back and forth, but often wonder if the horizontal space gained is worth the exception from community norm.

@lukeyeager lukeyeager changed the title [DON'T MERGE] Fix all pep8 lint Fix all pep8 lint Jan 27, 2017
@apaszke
Copy link
Contributor

apaszke commented Jan 27, 2017

I could never understand why is it 79, not 80...

@lukeyeager
Copy link
Contributor Author

why 120 vs 79 for column limit?

I like the GitHub rule: GitHub has a max line length of ~120 when displaying diffs.

Happy to change to whichever value the @pytorch folks like.

@soumith
Copy link
Member

soumith commented Jan 27, 2017

let's not discuss 120 vs 79 anymore :D
120 sounds fine, thanks.

@apaszke
Copy link
Contributor

apaszke commented Jan 27, 2017

@lukeyeager I've pushed a fix for a flaky Sqrt test and it's conflicting with your changes now. Can you reabse them once more please?

Here's the command I used to invoke autopep8 (in parallel!):

    git ls-files | grep '\.py$' | xargs -n1 -P`nproc` autopep8 -i

Several rules are ignored in setup.cfg. The goal is to let autopep8
handle everything which it can handle safely, and to disable any rules
which are tricky or controversial to address. We may want to come back
and re-enable some of these rules later, but I'm trying to make this
patch as safe as possible.

Also configures flake8 to match pep8's behavior.

Also configures TravisCI to check the whole project for lint.
@lukeyeager
Copy link
Contributor Author

Rebased. I discarded my conflicting changes, took yours, then ran autopep8 on top of them again.

@apaszke
Copy link
Contributor

apaszke commented Jan 27, 2017

@pytorchbot add to whitelist

@apaszke apaszke merged commit 79f5bf8 into pytorch:master Jan 28, 2017
@lukeyeager lukeyeager deleted the remaining-pep8 branch January 28, 2017 00:16
@apaszke
Copy link
Contributor

apaszke commented Jan 28, 2017

Thanks!

@apaszke
Copy link
Contributor

apaszke commented Jan 28, 2017

@lukeyeager Does the config have to be in setup.cfg? It's annoying because it creates a terminal autocomplete conflict with setup.py and that's used a lot.

@lukeyeager
Copy link
Contributor Author

lukeyeager commented Jan 28, 2017

I know, it bugs me too. Here are the other options:

  • ./tox.ini
  • ./.pep8 (deprecated)

So maybe tox.ini? I've seen major libraries use either one.

@apaszke
Copy link
Contributor

apaszke commented Jan 28, 2017

Hmmm ok, we're not using tox (SciPy is), so maybe let's keep it as is.

@ngimel
Copy link
Collaborator

ngimel commented Jan 28, 2017

@lukeyeager, would putting setup.cfg into tools directory and passing --config=tools to autopep8 be an option?

@lukeyeager
Copy link
Contributor Author

lukeyeager commented Jan 28, 2017

Good point, that's an option too. The advantage of keeping the config file in a standard location is that you can call pep8 . from anywhere in the tree and it will pick up the config for the project.

@apaszke
Copy link
Contributor

apaszke commented Jan 28, 2017

It's not that bad, let's keep it in the root for now. We'll have to start using pep8 a lot from now on.

@colesbury
Copy link
Member

If tox.ini works with pep8 let's just change the name. (It sounds like it's officially supported from the pep8 docs). The autocomplete for setup has been bugging me all day.

rohithkrn pushed a commit to rohithkrn/pytorch that referenced this pull request Apr 8, 2020
mrshenli pushed a commit to mrshenli/pytorch that referenced this pull request Apr 11, 2020
Enabling tensor index from random_split
mcarilli pushed a commit to mcarilli/pytorch that referenced this pull request Jan 27, 2021
KyleCZH pushed a commit to KyleCZH/pytorch that referenced this pull request Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PEP8

7 participants