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

setup isort & black, introduce pre-commit #331

Closed
wants to merge 2 commits into from

Conversation

syphar
Copy link
Contributor

@syphar syphar commented Jan 5, 2023

As discussed.

I kept the configuration & the actual reformat separate.

Of course we can discuss settings, or discard this completely if you don't like the format :)

@syphar
Copy link
Contributor Author

syphar commented Jan 5, 2023

btw, if feels like the readabilty could benefit from a smaller maximum line length (100?), but that's up to you

@rchl
Copy link
Contributor

rchl commented Jan 23, 2023

If the majority of the code uses single quotes already then don't you think that the configuration should be aligned with that to introduce least amount of noise to the PR and follow the preference of the main contributors?

@syphar
Copy link
Contributor Author

syphar commented Jan 23, 2023

that's a design question, and totally up to the contributors. Its configurable (via --skip-string-normalization).

IMO the main advantage of black is the opinionated style, but that's just me.

@rchl
Copy link
Contributor

rchl commented Jan 23, 2023

Another argument for single quotes could be that it's just easier to type on most popular keyboard layouts (US, EN) since it's just one key instead of two.

That said, if the quotes will be auto-fixed then maybe it doesn't matter. As you said, up to main contributors and I'm not one of them.

@ccordoba12
Copy link
Member

Closing as superseded by #419. Precommit can be added in another PR.

@ccordoba12 ccordoba12 closed this Aug 16, 2023
@syphar syphar deleted the isort branch August 17, 2023 08:53
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.

None yet

3 participants