Skip to content

PR: Setup pre-commit and format all code with Black#451

Merged
dalthviz merged 2 commits intospyder-ide:masterfrom
Czaki:pre-commit-setup-ci
Aug 28, 2023
Merged

PR: Setup pre-commit and format all code with Black#451
dalthviz merged 2 commits intospyder-ide:masterfrom
Czaki:pre-commit-setup-ci

Conversation

@Czaki
Copy link
Copy Markdown
Contributor

@Czaki Czaki commented Aug 25, 2023 •
edited
Loading

This PR setup pre commit with following hooks:

  • black - code formatter
  • ruff - statical code linter. Extremely fast, under active development that increases the number of rules. May need to revisit the list of rules to see if my selection (copied from napari) is ok. Here is a list of rules: https://beta.ruff.rs/docs/rules/ (selection is not full, ass we do not revisit it in napari for a long time). Main downside is that when pre-commit.ci makes PR with an update (set to monthly interval), there may be a need to introduce some fixes/blacklist some rules.
  • check-jsonschema check github workflows
  • setup-cfg-fmt - setup.cfg formatter
  • pre-commit/pre-commit-hooks - set of simple check for common simple mistakes like mixed line endings, validation json, yaml and toml for proper syntax (but not schema).

closes #345

@Czaki
Copy link
Copy Markdown
Contributor Author

Czaki commented Aug 25, 2023 •
edited
Loading

@dalthviz I think that best option for smooth experience is to setup pre-commit.ci on this repository.
Thom napari experience it provides nice autofix features if possible.

@Czaki Czaki marked this pull request as draft August 25, 2023 18:35
@dalthviz
Copy link
Copy Markdown
Member

dalthviz commented Aug 25, 2023 •
edited
Loading

Thank you so much for the help here @Czaki ! I think using pre-commit.ci makes sense 👍 I believe, probably after this get merged, that to enable it we just need to follow the web setup, right?

@dalthviz dalthviz added this to the v2.4.0 milestone Aug 25, 2023
@ccordoba12
Copy link
Copy Markdown
Member

ccordoba12 commented Aug 25, 2023 •
edited
Loading

Thanks for giving us a hand with this @Czaki!

One important thing to mention is that I think all changes in this PR should be squashed in a single commit and when everything is ready for merge, that commit should be added to .git-blame-ignore-revs, so we can still rely on git blame after this.

@dalthviz dalthviz mentioned this pull request Aug 25, 2023
Merged
@Czaki Czaki force-pushed the pre-commit-setup-ci branch from ded4236 to c26c552 Compare August 25, 2023 22:23
@Czaki Czaki marked this pull request as ready for review August 25, 2023 22:23
@Czaki
Copy link
Copy Markdown
Contributor Author

Czaki commented Aug 25, 2023

Squased. I hope that all imports that I removed by mistake are found.

@ccordoba12 ccordoba12 requested a review from dalthviz August 25, 2023 22:26
@ccordoba12 ccordoba12 changed the title setup pre-commit PR: Setup pre-commit and format all code with Black Aug 25, 2023
dalthviz
dalthviz reviewed Aug 25, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks for all the work @Czaki ! I left some comments regarding the new contribution guide pre-commit entry, and some code comments which seems like either need to be removed now or due to imports changing order need to be moved along side the new place the imports is.

Besides that I noticed a removed PySide6 import inside a test that maybe could be worthy to check locally but other than that this LGTM 👍 Again thank you so much for helping us with this!

@Czaki Czaki force-pushed the pre-commit-setup-ci branch from c26c552 to 4b12ece Compare August 26, 2023 17:19
dalthviz
dalthviz approved these changes Aug 28, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dalthviz dalthviz left a comment

Choose a reason for hiding this comment

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

Thanks @Czaki !

@dalthviz dalthviz merged commit 354461c into spyder-ide:master Aug 28, 2023
@Czaki Czaki deleted the pre-commit-setup-ci branch August 28, 2023 15:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dalthviz dalthviz dalthviz approved these changes

Assignees

@Czaki Czaki

Labels

type:Task

Projects

None yet

Milestone

v2.4.0

Development

Successfully merging this pull request may close these issues.

Use pre-commit to automatically format code with Black and imports with isort

3 participants

@Czaki @dalthviz @ccordoba12