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

🚨 Apply automatic formatting #474

Merged
merged 5 commits into from
Feb 29, 2024
Merged

🚨 Apply automatic formatting #474

merged 5 commits into from
Feb 29, 2024

Conversation

tdegeus
Copy link
Collaborator

@tdegeus tdegeus commented Feb 23, 2024

Fixes #464

@tdegeus tdegeus requested a review from MiWeiss February 23, 2024 16:20
Copy link
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

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

Thanks @tdegeus

Besides questions marked inline, one more remark: There's now a ton of rules to be enforced. Maybe a bit more than I would have chosen, but that's not necessarily a problem ;-) Instead, I worry that this makes the use of pre-commit essentially mandatory. This however must not happen - the hurdle for anyone to contribute should be as low as possible.

I won't object to the strict rules (there's much good to them, probably, after all), but I think we should provide simple rules to what everyone can execute locally to make their code compatible, where possible. (Something that would have been pip install -e .[lint] && black bibtexparser tests && isort bibtexparser tests --profile black or alike). A simple instruction in the contribution guideline on how to enable pre-commit (or at least a link to the corresponding docs) would probably not hurt either - again, just to make it as simple as possible for everyone.

.pre-commit-config.yaml Show resolved Hide resolved
setup.py Show resolved Hide resolved
@tdegeus
Copy link
Collaborator Author

tdegeus commented Feb 26, 2024

  • I would say that the key choice is to use black, that was and is the formatting rule.
  • The minor change I proposed is for isort, which is less key, and I'm open to negotiate.
  • The extra 'rule' is to truncate trailing whitespace everywhere, which is, I think, what most editors do, and therefore what results in the cleanest PRs.
  • The rest is mostly checks, such as small 'mistakes' that have been removed in rst and f-strings.

I could completely support supporting a second way to run these tools locally. It would be nice though to enforce keeping their settings in sync (which would be assured if we decide to go back to the default for isort, so I can easily support that). Running with pre-commit it's not hard either, though:

python -m pip install pre-commit
pre-commit run --all-files

and we should definitely document.
The advantage, I find, is that one does not have to keep track of which tools to run.

@MiWeiss
Copy link
Collaborator

MiWeiss commented Feb 26, 2024

Thanks for your comment. Sounds all good. I'll give the "formal" ( 😄 ) approval after the documentation update has been committed.

@MiWeiss MiWeiss changed the title Apply automatic formatting 🚨 Apply automatic formatting Feb 26, 2024
@tdegeus
Copy link
Collaborator Author

tdegeus commented Feb 28, 2024

Done (docs/contribute.rst). If you have comments / suggestions please feel free!

@MiWeiss
Copy link
Collaborator

MiWeiss commented Feb 28, 2024

Done (docs/contribute.rst). If you have comments / suggestions please feel free!

Thanks a lot. We now have two contribution docs: The one on readthedocs and the CONTRIBUTING.md in the git repo. How about we integrate the content of CONTRIBUTING.md on the one you newly created and just leave a link to the corresponding docs in the CONTRIBUTING.md. Note that we should not delete the CONTRIBUTING.md, as its presence is automatically detected by github and other tools.

@tdegeus
Copy link
Collaborator Author

tdegeus commented Feb 29, 2024

Thanks, sorry I overlooked that. Fixed.

Copy link
Collaborator

@MiWeiss MiWeiss left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks

@MiWeiss MiWeiss merged commit 53843c0 into sciunto-org:main Feb 29, 2024
18 checks passed
@tdegeus tdegeus deleted the format branch March 14, 2024 08:27
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.

Apply style using pre-commit
2 participants