Skip to content

/ Python

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

Jump to bottom

Add GitHub action for pre-commit #2515

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

@dhruvmanila dhruvmanila commented Sep 30, 2020

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
Add GitHub action file for pre-commit
Verified
This commit was signed with a verified signature.
dhruvmanila Dhruv Manilawala
GPG key ID: 9601911DE8797432 Learn about signing commits
Loading status checks…
e521b28
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020 •
edited

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:
Verified
This commit was signed with a verified signature.
dhruvmanila Dhruv Manilawala
GPG key ID: 9601911DE8797432 Learn about signing commits
Loading status checks…
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

@dhruvmanila dhruvmanila commented Sep 30, 2020

I think the name of the file should be lint, what do you think? @cclauss
@cclauss
cclauss reviewed Sep 30, 2020
View changes
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
@cclauss
cclauss requested changes Sep 30, 2020
View changes
Copy link
Member

@cclauss cclauss left a comment

Simplifying by always using the latest and greatest Python and by dropping names that are less self-documenting than the commands that they run.
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
.github/workflows/pre-commit.yml Outdated Show resolved Hide resolved
@cclauss
Copy link
Member

@cclauss cclauss commented Sep 30, 2020

Why not just add the shebang instead of removing the execution bit?
@cclauss
Copy link
Member

@cclauss 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
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e8edce4 
Co-authored-by: Christian Clauss <cclauss@me.com>
@dhruvmanila
Copy link
Member Author

@dhruvmanila dhruvmanila commented Sep 30, 2020

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 approved these changes Sep 30, 2020
View changes
@cclauss cclauss merged commit acaeb22 into TheAlgorithms:master Sep 30, 2020
3 checks passed
3 checks passed
codespell
Details
pre-commit
Details
Travis CI - Pull Request Build Passed
Details
@cclauss cclauss mentioned this pull request Sep 30, 2020
Merged
1 of 14 tasks complete
@dhruvmanila dhruvmanila deleted the dhruvmanila:github-action branch Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cclauss cclauss

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@dhruvmanila @cclauss
You can’t perform that action at this time.