search

GenericMappingTools / pygmt

A Python interface for the Generic Mapping Tools.
https://www.pygmt.org
BSD 3-Clause "New" or "Revised" License
747 stars 216 forks source link

Add pre-commit configuration file #1593

Closed willschlitzer closed 3 months ago

willschlitzer commented 2 years ago

To prevent the annoyance of realizing I forgot to run black after I have pushed a commit, I think it would be good to add a pre-commit hook configuration file to automatically run black, flake8, and other formatting packages.

Are you willing to help implement and maintain this feature? Yes

weiji14 commented 2 years ago

We'll need to discuss whether to add this. I recognize the value of using pre-commit, but I find that it is another layer of friction for new contributors to set up locally. Black is quite fast to run, but pylint can be very slow and I'd hate to run it every commit! We could also look at streamlining/improving the /format slash command if that's an issue.

willschlitzer commented 2 years ago

We'll need to discuss whether to add this. I recognize the value of using pre-commit, but I find that it is another layer of friction for new contributors to set up locally. Black is quite fast to run, but pylint can be very slow and I'd hate to run it every commit! We could also look at streamlining/improving the /format slash command if that's an issue.

I think of it as an optional add; contributors can choose to use pre-commit and already have a configuration file already to go. But I agree that making its usage mandatory is another barrier for entry for new contributors.

maxrjones commented 2 years ago

We'll need to discuss whether to add this. I recognize the value of using pre-commit, but I find that it is another layer of friction for new contributors to set up locally. Black is quite fast to run, but pylint can be very slow and I'd hate to run it every commit! We could also look at streamlining/improving the /format slash command if that's an issue.

Does the /format slash command not work for forks? I haven't had much success with using it lately.

willschlitzer commented 2 years ago

I tried this out on my local machine and it wasn't as user friendly as I hoped. I'm sure it can be configured to work alongside our make commands, but I'm going to close this issue since there doesn't seem to be much interest in it.

maxrjones commented 2 years ago

I've started using pre-commit while working on some other repositories and find it really nice. If there turns out to be interest from others in this tool, I could help set it up for the repository. I think it may be less friction for new contributors (install pre-commit and run pre-commit install once) than remembering the make commands given that it's getting to be fairly common.

weiji14 commented 2 years ago

Is it worth trying https://pre-commit.ci?

maxrjones commented 2 years ago

Is it worth trying https://pre-commit.ci/?

Yes, I think we could try this as an alternative to the format slash command and probably the style checks workflow.

seisman commented 1 year ago

I recognize the value of using pre-commit, but I find that it is another layer of friction for new contributors to set up locally. Black is quite fast to run, but pylint can be very slow and I'd hate to run it every commit!

I agree.

Is it worth trying https://pre-commit.ci/?

Yes, I think we could try this as an alternative to the format slash command and probably the style checks workflow.

The pre-commit.ci service commits changes directly to the branch that triggers the CI, which means that the PR authors must update their local branch (e.g., git pull) before pushing more changes to remote (i.e., git push), otherwise they will see errors like:

error: failed to push some refs to 'git@github.com:xxx/xxx.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

which are not friendly for git newbies.

In short, I'm inclined to not use pre-commit and close the issue.

maxrjones commented 1 year ago

The pre-commit.ci service commits changes directly to the branch that triggers the CI, which means that the PR authors must update their local branch (e.g., git pull) before pushing more changes to remote (i.e., git push), otherwise they will see errors like:

This is configurable and can be disabled so that the CI service doesn't push changes, but I'll close this since I was the one encouraging using pre-commit and haven't been working on PyGMT lately.

weiji14 commented 3 months ago

And... we're back! Continuing on from https://github.com/GenericMappingTools/pygmt/issues/3278, it sounds like the team is finally warming up to adding a .pre-commit-config.yaml file and installing the pre-commit.ci service.

Question: Do we want to replace the /format slash workflow with pre-commit.ci? Probably depends on whether pre-commit.ci works on forks (we can keep both first, and test to see how it works on forks).