search

camelot-dev / camelot

A Python library to extract tabular data from PDFs
https://camelot-py.readthedocs.io
MIT License
2.96k stars 466 forks source link

STY: Apply black #358

Closed MartinThoma closed 1 year ago

MartinThoma commented 1 year ago

Ran with:

pip install black==23.1.0
black .
bosd commented 1 year ago

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

foarsitter commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Close as completed by #353