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 to automatically fix style issue before pushing commits #973

Merged
merged 4 commits into from
Sep 11, 2023

Conversation

eurunuela
Copy link
Collaborator

Closes #972.

This will be a draft until other tedana devs share their opinion and approve in #972.

Changes proposed in this pull request:

  • Adds pre-commit config file
  • Adds pre-commit dependency for developers
  • Mentions the use of pre-commit in the contributing guidelines

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (bf3392a) 88.86% compared to head (b225bc4) 88.86%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #973   +/-   ##
=======================================
  Coverage   88.86%   88.86%           
=======================================
  Files          27       27           
  Lines        3368     3368           
  Branches      618      618           
=======================================
  Hits         2993     2993           
  Misses        227      227           
  Partials      148      148           

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

@eurunuela eurunuela marked this pull request as ready for review August 29, 2023 20:02
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

You removed a dead link, but I think we should include the correct link somewhere.

Here's my suggestion:

Current line 44:
We also maintain a [Mattermost chat room][link_mattermost] for more informal conversations and general project updates.

Suggested revision:
We also maintain a [Mattermost chat room][link_mattermost] for more informal conversations and general project updates. Our general documentation also includes additional information on contributing, development philosophy, and our monthly developer calls.

(BTW, is there a way for me to make a suggested changes on lines of code that weren't revised by you? I could open a PR to your PR, but, for one-off things like this, is what I'm doing here the easiest approach?)

I also don't know how to evaluate the core functionality of this PR since I haven't worked with pre-commits before. If @tsalo has & is happy with this, I can approve. Otherwise, maybe @emdupre could take a quick look.

CONTRIBUTING.md Outdated Show resolved Hide resolved
@eurunuela
Copy link
Collaborator Author

I'm sorry @handwerkerd, it seems like autoformatting on save changed all those things. I just reverted and saved the changes with no autoformatting this time.

handwerkerd
handwerkerd previously approved these changes Sep 8, 2023
Copy link
Member

@handwerkerd handwerkerd left a comment

Choose a reason for hiding this comment

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

LGTM.
I hope the other reviewer is someone who has used pre-commits before and can confirm this is set up correctly.

CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Taylor Salo <tsalo90@gmail.com>
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

LGTM

@eurunuela
Copy link
Collaborator Author

Thank you for the feedback Taylor!

@eurunuela eurunuela merged commit f9daaa0 into ME-ICA:main Sep 11, 2023
2 checks passed
@eurunuela eurunuela deleted the precommit branch September 12, 2023 08:31
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.

Proposal to add pre-commits to tedana
3 participants