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

pre-commit #195

Open
eggplants opened this issue Jan 14, 2022 · 6 comments
Open

pre-commit #195

eggplants opened this issue Jan 14, 2022 · 6 comments

Comments

@eggplants
Copy link
Contributor

#184 (comment)

I think what is needed:

  • format with black
  • lint with flake8
  • sort import statements with isort
  • (lint with mypy after Typing #193)
  • ...
@eggplants
Copy link
Contributor Author

@aucampia Are you going to prepare for pre-commit CI?

@nicholascar
Copy link
Member

Yes, I think we are fine with pre-commit. Can you please use exactly the same black & flake8 settings are rdflib though? You will see in there that there is a fixed parameters, version-locked black as well as a Flak8 with a few settings specified.

@aucampia
Copy link
Member

aucampia commented Jan 16, 2022

@eggplants I made a PR for adding pre-commit to https://github.com/RDFLib/rdflib and enabled pre-commit CI for it.

If you make a PR for adding pre-commit config here here I will enable pre-commit CI for this repo also.

Regarding flake8, we still need to rationalize our config for https://github.com/RDFLib/rdflib - some work was started on this in RDFLib/rdflib#1442 but we have to finish it. So for now I'm not adding it because it will always fail. You can add it but please try and keep PRs small to make it easier to review, enabling all checks for flake8 at once and fixing all their issues at once is maybe going too far, but we will do our best with reviews.

The regarding mypy, I did not add it to pre-commit as we run this as part of tox and our CI, I'm open to other views here, but since tests are separate anyway I don't see much benefit to having mypy in pre-commmit as opposed to in tox.

@aucampia
Copy link
Member

For reference, PR on RDFLib/rdflib is:

@aucampia
Copy link
Member

One of the main benefits of pre-commit.ci for me is the ability to do pre-commit.ci autofix in PRs, so what I put in there is mainly geared towards things that can be automatically fixed, like isort, pycln and black.

@eggplants
Copy link
Contributor Author

@aucampia Could you install pre-commit in sparqlwrapper as well as rdflib?

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 a pull request may close this issue.

3 participants