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 hook for TheAlgorithms/Python #2511

Merged
merged 5 commits into from Sep 30, 2020

Conversation

@dhruvmanila
Copy link
Member

@dhruvmanila dhruvmanila commented Sep 30, 2020

As mentioned in #2509 (comment)

Describe your change:

  • Add an algorithm?

Checklist:

  • I have read CONTRIBUTING.md.
  • This pull request is all my own work -- I have not plagiarized.
  • I know that pull requests will not be merged if they fail the automated tests.
  • This PR only changes one algorithm file. To ease review, please open separate PRs for separate algorithms.
  • All filenames are in all lowercase characters with no spaces or dashes.
  • If this pull request resolves one or more open issues then the commit message contains Fixes: #{$ISSUE_NO}.
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

There are a bunch of files that will be fixed if I run this.
Should I open another PR for those fixes?

Also, take a look at https://github.com/asottile/reorder_python_imports
This seems to be a better alternative to isort in terms of being a public repository.

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

We also need to inform people to enable it in their local copy by:

$ pip install pre-commit
$ pre-commit install

Also, there are badges for pre-commit and black.
Shall I add them?

Copy link
Member

@cclauss cclauss left a comment

This is awesome! How long does it take to run on your machine? (If it is not fast, contributors will turn it off.) Please add the badges to README.md in a separate follow-up PR. Please fix the issues in a separate PR that we should land before this one.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Show resolved Hide resolved
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

By default, pre-commit runs only on the new files or modified files. We can change it to run on all the files all the time but that would be inefficient.

Also, the environment is installed only when it is run the first time and when the rev is changed (versions are changed).

@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

OK. Yes now I understand. After this lands, we can add something like this to pre-commit run --all-files https://github.com/cclauss/itinerant-tester/blob/master/.github/workflows/pre-commit.yml

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

I don't think so we need a workflow file for this as all this is done locally. The workflow file is run after the commit has been pushed and mainly used to check if every test passed. It's mostly just for the visuals as far as I can understand.

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

TODO:

  • Separate PR for all the fixes BEFORE this is merged
  • Follow-up PR for adding badges to README.md
  • Follow-up PR for a GitHub Action to pre-commit run --all-files

Is that correct?

@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

A fraction of all contributors will install and run pre-commit. The workflow would check the work of the contributors who do not install the tool and it will allow us to remove all that complexity from .travis.yml which should speed up our test runs.

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

Got it!

@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

Do you have rights to click the Squash and merge button above?

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

Missed it! But I don't think so as that will be available only to members and owners.

Oh, it's still not merged then, no I don't have the rights.

@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

OK. We need the fix PR before we merge.

Also I added to the GitHub Action to your task list above.

@dhruvmanila dhruvmanila mentioned this pull request Sep 30, 2020
5 of 5 tasks complete
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

@cclauss DON'T MERGE THIS YET.
We need to ignore some files which require whitespace and newlines.

@dhruvmanila dhruvmanila requested a review from cclauss Sep 30, 2020
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

@cclauss Please take a look at the changes:
EOF newline will be checked only for Python files and ignoring whitespace for that specific file as it requires it in pytest

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

I can see the merge option on the app but not on the browser.

@cclauss cclauss merged commit ae65f55 into TheAlgorithms:master Sep 30, 2020
2 checks passed
2 checks passed
codespell
Details
Travis CI - Pull Request Build Passed
Details
@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

THANKS!!

@dhruvmanila dhruvmanila deleted the dhruvmanila:pre-commit-hook branch Sep 30, 2020
@cclauss cclauss mentioned this pull request Sep 30, 2020
1 of 14 tasks complete
@DmytroLitvinov
Copy link
Contributor

@DmytroLitvinov DmytroLitvinov commented Oct 5, 2020

Hi @dhruvmanila ,

I guess we should warn peope about pre-commit at CONTRIBUTING.md file.

@cclauss
Copy link
Member

@cclauss cclauss commented Oct 5, 2020

Let's not force the complexities of a local install of pre-commit on all contributors. Some folks are pretty junior and adding pre-commit to their machines might turn them off from contributing. The README should however show them how to find and solve issues when the GitHub Action does not love their contribution.

@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Oct 5, 2020

Maybe we should put it in Travis_CI_tests_are_failing and change the name of the file to tests_are_failing and put the instructions as to how to find the error for all the automatic tests we're performing in that file.

The other idea would be to separate out the files for each test but having a lot of files doesn't seem attractive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.