Skip to content

Conversation

@dwalton76
Copy link
Collaborator

No description provided.

@dwalton76
Copy link
Collaborator Author

Hmm this is a very large patch...9200 lines

@WasabiFan
Copy link
Member

Wow, yeah, that's pretty big!

Although I am rather fond of keeping style consistent and using these tools, I feel like in this case the sheer amount of stuff that would change makes it not worth it. It makes blames/diffs/history searching hard for a relatively small benefit. I think we should leave the formatting inconsistent unless there's another compelling reason to change so much.

@dwalton76
Copy link
Collaborator Author

On the flip side...

  • if we are ever going to do it now (before we pull an ev3dev-buster branch) is the time
  • once we rip off this bandaid and make the style consistent we could use a precommit hook to keep it this way (make black --check pass in order for the commit to be allowed).

I could go either way but am leaning towards "lets do it and be done with it".

@dlech
Copy link
Member

dlech commented Jan 13, 2020

Or use yapf instead of black and stick with single quotes

@dwalton76
Copy link
Collaborator Author

I hadn't heard of yapf but it looks worth a try. I'll post another PR using yapf and we can compare the diff for it vs black.

@dwalton76 dwalton76 changed the title use double quotes consistently (ran black) format code via black Jan 15, 2020
@dwalton76
Copy link
Collaborator Author

For comparison the yapf patch is 4300 lines...so still significant
#716

@WasabiFan
Copy link
Member

OK, I'm up for formatting. I do prefer the yapf results just because it modifies less code.

If that's the one we're going to go with, can you add it (asserting matching style) to the TravisCI scripts? It can probably go in the CPython-only build (as opposed to micropython).

@dwalton76
Copy link
Collaborator Author

If that's the one we're going to go with, can you add it (asserting matching style) to the TravisCI scripts? It can probably go in the CPython-only build (as opposed to micropython).

We should be able to do a pre-commit hook that verifies the style is correct, that way the user can't even commit it if the style is wrong.

@WasabiFan
Copy link
Member

I generally prefer to avoid commit hooks... Commits are free and easily squashable backups, and I don't want something like a style check to get in the way of that. Being able to quickly push half-baked changes then pull them from another machine and amend your work there is very useful. I'd also rather not have to fight cross-platform scripting issues.

@dwalton76
Copy link
Collaborator Author

We formatted via yapf instead

@dwalton76 dwalton76 closed this Jan 20, 2020
@dwalton76 dwalton76 deleted the black branch January 20, 2020 00:48
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.

3 participants