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_fixes #115

Closed
wants to merge 1 commit into from
Closed

PEP8_fixes #115

wants to merge 1 commit into from

Conversation

the-zebulan
Copy link
Contributor

Fixed some PEP8 issues pointed out by CodeClimate

  • Continuation line under-indented for visual indent (multiple)
  • Continuation line over-indented for visual indent (multiple)
  • Line too long (multiple)
  • The backslash is redundant between brackets (multiple)
  • Missing whitespace around operator
  • Continuation line with same indent as next logical line (multiple)

Initial PR only modififed chord.py so some of the style choices around indentation and brackets instead of backslashes could be discussed/approved.

@the-zebulan
Copy link
Contributor Author

I should have run CodeClimate on my fork first (I just did it now), I guess it doesn't like the line breaks before an operator!

I was reading PEP8 and thought it was the right thing to do but I can change them back.

https://www.python.org/dev/peps/pep-0008/#should-a-line-break-before-or-after-a-binary-operator

@LivInTheLookingGlass
Copy link
Collaborator

I really appreciate all the effort you've put in. If I'd caught you earlier I would have warned that chord.py has a lot of churn to go through before it's stable, though. I'll probably merge this after I go through it, but I would definitely concentrate more on the other files before I looked here.

#103 and #106 are still mission critical for that file, and I don't know how to translate the other implementations I've seen to work here yet. I got close a couple times, and the current setup is mostly stable for small networks, but still.

@LivInTheLookingGlass
Copy link
Collaborator

Seriously though, it's awesome that you did this.

@the-zebulan
Copy link
Contributor Author

the-zebulan commented Nov 29, 2016

Oh, my mistake. Do you think I should just delete this PR or maybe just revert chord.py and keep this PR open but do the other files instead?

It was just the first file in on the "Hotspots" list on CodeClimate, that's the only reason I did it first.

@LivInTheLookingGlass
Copy link
Collaborator

Either way is fine with me. If you'd rather, I'll take a look at chord.py and see if those changes should be kept. That way we waste as little effort as possible. Feels too much like looking a gift horse in the mouth otherwise. It needs to happen eventually anyhow.

@the-zebulan
Copy link
Contributor Author

I don't mind closing this one and starting a new PR with the other files instead. I don't want to cause any unnecessary work for you, I just wanted to help out (very small way but still). Also, I find it amusing to get rid of PEP8 warnings in PyCharm, it's almost a game for me!

@LivInTheLookingGlass
Copy link
Collaborator

Haha, fair enough

@the-zebulan the-zebulan deleted the PEP8_fixes branch November 29, 2016 04:22
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.

2 participants