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

[DO NOT MERGE] Autoformat using black (alternative to #66) #56

Closed
wants to merge 3 commits into from

Conversation

hartwork
Copy link
Member

@hartwork hartwork commented Jan 1, 2021

Needs (more or less) all other pull requests to have been merged to not cause painful conflicts.
The idea is to present an earlier version for review now so that I can work on more things concurrently.
Sits on top of pull request #53 to avoid conflicts, that will go away later as well.

PS: For the autoformat commit, maybe an author like black <black@autoformatter.invalid> works best. Happy about other suggestions.

Alternative to #66

@hartwork hartwork requested a review from esc January 1, 2021 17:14
@hartwork hartwork force-pushed the autoformat-using-black branch from bdf418b to 0f4e6a3 Compare January 1, 2021 22:42
@hartwork hartwork changed the title [DO NOT MERGE] Autoformat using black [DO NOT MERGE] Autoformat using black (alternative to #66) Jan 6, 2021
@hartwork
Copy link
Member Author

hartwork commented Jan 6, 2021

@esc after comparing the results of black here and payf in #66 my impression is that black takes a lot more vertical space than seems healthy and needed to me and that yapf is a lot less invasive while still moving us into the 99%-pep8-conformant zone where we mostly can stop thinking about formatting. Are you a strong proponent of black for this code base? I'm tending towards voting for payf over black myself at the moment. (Either would be too early to merge but I think they already serve to give an impression.)

@hartwork hartwork mentioned this pull request Jan 11, 2021
4 tasks
@esc
Copy link
Contributor

esc commented Jan 13, 2021

I'm in two minds about this.

yapf:

    format_group.add_argument('-g',
                              '--graphviz',
                              default=None,
                              action='store_true',
                              dest=GRAPHVIZ,
                              help='output lines suitable as input for dot/graphviz')

vs. black:


    format_group.add_option(
        '-g',
        '--graphviz',
        action='store_true',
        dest='graphviz',
        help='output lines suitable as input for dot/graphviz',
    )

So, for this particular example, I would favour black

@esc
Copy link
Contributor

esc commented Jan 13, 2021

But you are right, yapf seems a lot less invasive, especially in the comments.

@esc
Copy link
Contributor

esc commented Jan 13, 2021

I'm fine with either actually, but also leaning towards yapf, for the same reasons you cite above.

@hartwork
Copy link
Member Author

Awesome, let's go for yapf then! Closing the black one here now.

@hartwork hartwork closed this Jan 13, 2021
@hartwork hartwork deleted the autoformat-using-black branch January 13, 2021 14:59
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.

2 participants