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 #1507

Merged
merged 3 commits into from Oct 24, 2021
Merged

Conversation

phschiele
Copy link
Collaborator

This PR adds a config file for pre-commit to reduce the number of "Fix flake8/isort" commits and make developing easier :)

Pre-commits can perform checks on the diff whenever git commit is invoked, e.g. the proposed config will check the two conventions currently enforce by the linters, i.e. isort/flake8.

Quick start

pip install pre-commit
pre-commit install

Example usage

... make some change to "cvxpy/foo.py", not following the enforced conventions

git add cvxpy/foo.py
git commit -m "new feature"

-> Pre-commit will check for violations and abort the commit if any are present
It will even automatically fix them in some cases (e.g. isort)
image

git add cvxpy/foo.py  # Add the file again after fixing it
git commit -m "new feature"  # This time the commit goes through

image

Notes:

  • If you think this is too specific to an individual workflow, feel free to close this PR and I will simply keep the config locally
  • The hooks can be ignored by adding the -n / --no-verify flag to the commit command
  • Using the hooks is of course fully optional, i.e. it should not be an additional hurdle for contributons
  • Further hooks can easily be added if desired (e.g. black, mypy, unimport, ...)

@rileyjmurray
Copy link
Collaborator

@phschiele I love the idea of something to catch style issues before a push/PR. I think there's no harm in adding a config file that only developers interact with (and that they have to opt-in to).

It would be good to mention that this is an option on the contributing docs. Could you update the text at https://www.cvxpy.org/contributing/index.html#code-style to suggest using pre-commit? The language should be clear that this is strictly optional.

@phschiele
Copy link
Collaborator Author

@phschiele I love the idea of something to catch style issues before a push/PR. I think there's no harm in adding a config file that only developers interact with (and that they have to opt-in to).

It would be good to mention that this is an option on the contributing docs. Could you update the text at https://www.cvxpy.org/contributing/index.html#code-style to suggest using pre-commit? The language should be clear that this is strictly optional.

Done in 8d5ff27

@rileyjmurray
Copy link
Collaborator

Looks good to me! But I'll give @SteveDiamond and @akshayka a chance to chime in.

Copy link
Collaborator

@akshayka akshayka left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@SteveDiamond SteveDiamond merged commit 1353bd0 into cvxpy:master Oct 24, 2021
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.

None yet

4 participants