pygmt icon indicating copy to clipboard operation
pygmt copied to clipboard

Add pre-commit configuration file

Open willschlitzer opened this issue 4 years ago • 7 comments

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

willschlitzer avatar Oct 28 '21 21:10 willschlitzer

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.

weiji14 avatar Oct 28 '21 21:10 weiji14

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.

willschlitzer avatar Oct 29 '21 04:10 willschlitzer

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.

maxrjones avatar Nov 01 '21 16:11 maxrjones

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.

willschlitzer avatar Nov 19 '21 07:11 willschlitzer

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.

maxrjones avatar Aug 12 '22 17:08 maxrjones

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

weiji14 avatar Aug 17 '22 17:08 weiji14

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.

maxrjones avatar Aug 17 '22 21:08 maxrjones

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 '[email protected]: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.

seisman avatar Jun 14 '23 08:06 seisman

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.

maxrjones avatar Jun 14 '23 13:06 maxrjones