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

Black linting #639

Merged
merged 10 commits into from
Nov 3, 2023
Merged

Black linting #639

merged 10 commits into from
Nov 3, 2023

Conversation

zerothi
Copy link
Owner

@zerothi zerothi commented Nov 3, 2023

I have long wanted to do some kind of linting.
Playing round turns out that there is no tool that really suits every wish.

I think it would be beneficial for sisl to use a linter to reduce the pain of going through code styles which is somewhat subjective, and also turns out to be too time-consuming.

With this PR we enforce the black coding style, whether we like it or not.

Comments would be greatly appreciated.

tool-setup in pyproject.toml, and fixed
Periodic table to not be formatted.

Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Copy link

codecov bot commented Nov 3, 2023

Codecov Report

Attention: 354 lines in your changes are missing coverage. Please review.

Comparison is base (89dcede) 87.71% compared to head (80edad5) 87.73%.
Report is 7 commits behind head on main.

❗ Current head 80edad5 differs from pull request most recent head d79400d. Consider uploading reports for the commit d79400d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
+ Coverage   87.71%   87.73%   +0.01%     
==========================================
  Files         362      362              
  Lines       48354    48436      +82     
==========================================
+ Hits        42415    42496      +81     
- Misses       5939     5940       +1     
Files Coverage Δ
src/sisl/__init__.py 98.00% <100.00%> (ø)
src/sisl/_array.py 100.00% <100.00%> (ø)
src/sisl/_common.py 100.00% <ø> (ø)
src/sisl/_environ.py 87.17% <100.00%> (ø)
src/sisl/_internal.py 100.00% <ø> (ø)
src/sisl/_typing.py 100.00% <100.00%> (+100.00%) ⬆️
src/sisl/_typing_ext/numpy.py 50.00% <100.00%> (ø)
src/sisl/geom/_common.py 100.00% <100.00%> (ø)
src/sisl/geom/basic.py 97.10% <100.00%> (ø)
src/sisl/geom/bilayer.py 98.68% <100.00%> (+0.01%) ⬆️
... and 236 more

... and 65 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Nick Papior <nickpapior@gmail.com>
Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

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

CodeQL found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

Could you add this and isort to the PR CI? So that you don't need to run them locally and therefore we don't forget :)

@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

Maybe using pre-commit? https://pre-commit.com/#intro

@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

And actually if pre-commit is installed locally it's a great tool because it will run everything automatically when you do git commit.

Signed-off-by: Nick Papior <nickpapior@gmail.com>
@tfrederiksen
Copy link
Contributor

Looks good to me!

Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
Signed-off-by: Nick Papior <nickpapior@gmail.com>
@zerothi
Copy link
Owner Author

zerothi commented Nov 3, 2023

I have tried to make the test depend on the linter, this seems to not work since it is not part of the main branch.
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

Signed-off-by: Nick Papior <nickpapior@gmail.com>
@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

I'm just checking pre-commit for another package and it works great with black, and probably isort too.

All you have to do is to have a .pre-commit-config.yaml file on the root folder which looks like:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.5.0
    hooks:
    -   id: check-yaml
    -   id: end-of-file-fixer
    -   id: trailing-whitespace
# Using this mirror lets us use mypyc-compiled black, which is about 2x faster
- repo: https://github.com/psf/black-pre-commit-mirror
  rev: 23.10.1
  hooks:
    - id: black-jupyter

Then run pip install pre-commit and pre-commit install. Then each time you run git commit everything runs and the fixes are added to the commit.

It is much better than handling each fixer manually, give it a try :)

Then the checkbox can simply be removed, and run pre-commit on the CI to check that everything is fine, optionally automatically commiting the fixes.

@zerothi zerothi merged commit 07359d8 into main Nov 3, 2023
2 checks passed
@zerothi zerothi deleted the black branch November 3, 2023 12:23
@zerothi
Copy link
Owner Author

zerothi commented Nov 3, 2023

I'll merge and continue :)

I'm just checking pre-commit for another package and it works great with black, and probably isort too.

All you have to do is to have a .pre-commit-config.yaml file on the root folder which looks like:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.5.0
    hooks:
    -   id: check-yaml
    -   id: end-of-file-fixer
    -   id: trailing-whitespace
# Using this mirror lets us use mypyc-compiled black, which is about 2x faster
- repo: https://github.com/psf/black-pre-commit-mirror
  rev: 23.10.1
  hooks:
    - id: black-jupyter

Then run pip install pre-commit and pre-commit install. Then each time you run git commit everything runs and the fixes are added to the commit.

It is much better than handling each fixer manually, give it a try :)

Then the checkbox can simply be removed, and run pre-commit on the CI to check that everything is fine, optionally automatically commiting the fixes.

I think this is a great advice, if anything we could encourage users to do it, but since this package is still in its infancy int terms of adoption, I would be afraid that it scares people away. If people find this useful, then use it locally, it will help us, but it shouldn't be a barrier for new devs :)

@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

I think the separate fixers is more of a barrier than pre-commit, with the checkbox "Ran X, Y, ... at top level" being scary.

And the point is that in the end pre-commit should run on CI to make sure everything is fine, so the devs are not really required to use it.

@zerothi
Copy link
Owner Author

zerothi commented Nov 3, 2023

I think the separate fixers is more of a barrier than pre-commit, with the checkbox "Ran X, Y, ... at top level" being scary.

And the point is that in the end pre-commit should run on CI to make sure everything is fine, so the devs are not really required to use it.

Ah, you want that, that could also be a way out.

@pfebrer
Copy link
Contributor

pfebrer commented Nov 3, 2023

I think this is a great advice, if anything we could encourage users to do it

By the way having both approaches is possible. You just have the .pre-commit-config.yaml file in the root of the repo in case anyone wants to do it with pre-commit, but it can still be done manually.

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.

Using black code formatting style
3 participants