Skip to content

Add GitHub action for pre-commit #2515

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 30, 2020
Merged

Add GitHub action for pre-commit #2515

merged 3 commits into from
Sep 30, 2020

Conversation

dhruvmanila
Copy link
Member

Follow-up on #2511
As mentioned in #2511 (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.

@dhruvmanila
Copy link
Member Author

dhruvmanila commented Sep 30, 2020 •
edited
Loading

It's working perfectly: https://github.com/TheAlgorithms/Python/pull/2515/checks?check_run_id=1186742542

A few small changes required:

  • Fix instagram_crawler.py using black
  • Either add shebang or remove the executable bit from those files

With this in place, we can remove black, flake8, validate_filenames.py step from Travis CI and it needs to check only with pytest.

@dhruvmanila
Fix errors exposed by pre-commit hook:
d6a7879 
- Remove executable bit from files without shebang. I checked those
  file and it was not needed.
- Fix with black
@dhruvmanila
Copy link
Member Author

I think the name of the file should be lint, what do you think? @cclauss

cclauss
cclauss requested changes Sep 30, 2020
View reviewed changes
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

Simplifying by always using the latest and greatest Python and by dropping names that are less self-documenting than the commands that they run.

@cclauss
Copy link
Member

cclauss commented Sep 30, 2020

Why not just add the shebang instead of removing the execution bit?

@cclauss
Copy link
Member

cclauss commented Sep 30, 2020

I think the name of the file should be lint, what do you think? @cclauss

I think the current name is good enough. The script runs pre-commit so it should be called pre-commit.

@dhruvmanila @cclauss
Apply suggestions from code review
e8edce4 
Co-authored-by: Christian Clauss <[email protected]>
@dhruvmanila
Copy link
Member Author

Except for the scheduling/round_robin.py file, other files don't output anything when I run python <filename> so I thought there's no point in keeping the executable bit on.

If you think otherwise, I will turn them on?

@cclauss cclauss merged commit acaeb22 into TheAlgorithms:master Sep 30, 2020
@cclauss cclauss mentioned this pull request Sep 30, 2020
Merged
14 tasks
@dhruvmanila dhruvmanila deleted the github-action branch October 1, 2020 02:05
stokhos pushed a commit to stokhos/Python that referenced this pull request Jan 3, 2021
@dhruvmanila @cclauss @stokhos
Add GitHub action for pre-commit (TheAlgorithms#2515)
5012146 
* Add GitHub action file for pre-commit

* Fix errors exposed by pre-commit hook:

- Remove executable bit from files without shebang. I checked those
  file and it was not needed.
- Fix with black

* Apply suggestions from code review

Co-authored-by: Christian Clauss <[email protected]>

Co-authored-by: Christian Clauss <[email protected]>
Panquesito7 pushed a commit to Panquesito7/Python that referenced this pull request May 13, 2021
@dhruvmanila @cclauss
Add GitHub action for pre-commit (TheAlgorithms#2515)
2acd450 
* Add GitHub action file for pre-commit

* Fix errors exposed by pre-commit hook:

- Remove executable bit from files without shebang. I checked those
  file and it was not needed.
- Fix with black

* Apply suggestions from code review

Co-authored-by: Christian Clauss <[email protected]>

Co-authored-by: Christian Clauss <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss cclauss approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhruvmanila @cclauss