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

Add .pre-commit-config.yaml #67

Merged
merged 15 commits into from
Jun 6, 2024
Merged

Conversation

maestroque
Copy link
Contributor

Add a pre-commit configuration file, as in phys2bids

Proposed Changes

  • All the python style checkers are added
  • rst checkers are added as is from phys2bids

Change Type

  • bugfix (+0.0.1)
  • minor (+0.1.0)
  • major (+1.0.0)
  • refactoring (no version update)
  • test (no version update)
  • infrastructure (no version update)
  • documentation (no version update)
  • other

Checklist before review

  • I added everything I wanted to add to this PR.
  • [Code or tests only] I wrote/updated the necessary docstrings.
  • [Code or tests only] I ran and passed tests locally.
  • [Documentation only] I built the docs locally.
  • My contribution is harmonious with the rest of the code: I'm not introducing repetitions.
  • My code respects the adopted style, especially linting conventions.
  • The title of this PR is explanatory on its own, enough to be understood as part of a changelog.
  • I added or indicated the right labels.
  • I added information regarding the timeline of completion for this PR.
  • Please, comment on my PR while it's a draft and give me feedback on the development!

@maestroque maestroque self-assigned this May 27, 2024
Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

I see some formatting has happened ;)

The main feedback is to change the setup.cfg configuration - you can take a look at phys2bids' or nigsp's one to see how to set pre-commit and its hooks up!

.github/ISSUE_TEMPLATE/ISSUE_TEMPLATE_DISCUSSION.md Outdated Show resolved Hide resolved
.github/ISSUE_TEMPLATE/ISSUE_TEMPLATE_MEETING.md Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
setup.cfg Show resolved Hide resolved
@smoia
Copy link
Member

smoia commented May 27, 2024

This may benefit from having a look at #41 (?)

peakdet/operations.py Outdated Show resolved Hide resolved
@maestroque
Copy link
Contributor Author

@smoia @me-pic @m-miedema I believe this is ready, feel free to have a look

@maestroque
Copy link
Contributor Author

@me-pic @m-miedema @smoia This should be ready to merge. I opened physiopy/prep4phys#2 for the failing neurokit tests, since they are outdated and I suppose they will be addressed in another PR (or will be re-written completely). For the time being I just added a # noqa

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Looks like the changes worked, good job!

Did you run tests locally? If not, could you do that, please? If they break on something like the old workflows, that's fine, but otherwise it's good to check that the blacking of the scripts didn't break anything (shouldn't, but just to be sure!)

write-changes =
count =
quiet-level = 3

Copy link
Member

@smoia smoia Jun 4, 2024

Choose a reason for hiding this comment

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

Might be worth adding black config as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no support for setup.cfg (issue), we would have to add a pyproject.toml instead.
I don't see any desired configuration option other than perhaps the --exclude option.

However, I don't see any real benefit in excluding files like versioneer.py and not having style uniformity. Let me know your thoughts.

@maestroque
Copy link
Contributor Author

@smoia I ran them locally yes, they have the same behavior as in master branch currently.

setup.cfg Outdated
tests
versioneer.py
ignore = E126, E203, E402, W503
max-line-length = 99
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, you are right about black in setup.cfg.

I got confused with extended Flake8 settings to match Black:

ignore = E126, E402, W503, F401, F811
max-line-length = 88
extend-ignore = E203, E501
extend-select = B950

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@smoia
Copy link
Member

smoia commented Jun 4, 2024

@smoia I ran them locally yes, they have the same behavior as in master branch currently.

This is awesome though!

@maestroque
Copy link
Contributor Author

@smoia @m-miedema @me-pic this should be ready to go now

Copy link
Member

@smoia smoia left a comment

Choose a reason for hiding this comment

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

Awesome!

@smoia smoia merged commit be45064 into physiopy:master Jun 6, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants