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.

Sign up for GitHub

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

Jump to bottom

STY: Apply black #358

Closed
wants to merge 2 commits into from
Closed

STY: Apply black #358

wants to merge 2 commits into from

Conversation

MartinThoma
Copy link
Contributor

Ran with:

pip install black==23.1.0
black .

@MartinThoma
STY: Apply black
bbcce34 
Ran with:
    $ pip install black==23.1.0
    $ black .
@MartinThoma MartinThoma requested a review from a team March 11, 2023 12:42
@MartinThoma MartinThoma mentioned this pull request Mar 11, 2023
Merged
bosd
bosd approved these changes Jul 14, 2023
View reviewed changes
Copy link

@bosd bosd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bosd
Copy link

bosd commented Jul 14, 2023

@foarsitter Do you have the super powers to re-trigger the gh actions on this one?

@foarsitter
Copy link
Contributor

@bosd we need a stronge fundament since we cannot rely on the test-suite. I tink we need to move forward and merge #353. Is that something you can review?

@bosd
Copy link

bosd commented Jul 15, 2023

@foarsitter I came to this pr from #353. As this small incremental PR was easier to review.
Made sense to have black in a seperate PR.

@foarsitter
Copy link
Contributor

Sure it makes sense, but for me it makes no sense to review a PR of something I already did in combination with fixing the fundament. Hard decisions need to be made in order to get this project on track.

Applying black without fixing the pdftopng problem is like painting a car that is broken. Sure, it is nice it looks good, but it stil doesn't drive.

@bosd
Copy link

bosd commented Jul 15, 2023

I agree,
Actually the other PR was not that hard to dig true, as some parts of it I've already reviewed in separate PR's.

IMO, better to get the flow of contributions and PR's going.
If we merge something that breaks, we can fix it..
As long as we make sure, the published releases are somewhat stable.

Let's get this project back on track.

@MartinThoma
Copy link
Contributor Author

I'm not sure if I follow this discussion properly, so I'll just share my 2ct on this PR:

  1. This PR should be trivial to merge, but only be done after other big changes are done.
  2. This PR should not be rebased. Instead, it would be better to just apply the commands from above (maybe with a newer black version) on the laster master branch.
  3. Formatting changes like this should always be done in a separate PR, as they hide other changes (although this one is surprisingly small ... so I guess black was applied before?)
  4. I personally like having a pre-commit hook that applies black + a CI (or a maintainer) who makes sure that it is applied regularly. Not necessarily in every PR as I wouldn't block a valuable merge just because of formatting, but in such cases right after the merge.

TL;DR: Feel free to close this PR + apply it again on master :-)

@foarsitter
Copy link
Contributor

Close as completed by #353

@foarsitter foarsitter closed this Sep 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bosd bosd bosd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@MartinThoma @bosd @foarsitter