Skip to content

Add GitHub action for pre-commit - #2515

Merged
cclauss merged 3 commits into
TheAlgorithms:masterfrom
dhruvmanila:github-action
Sep 30, 2020
Merged

Add GitHub action for pre-commit#2515
cclauss merged 3 commits into
TheAlgorithms:masterfrom
dhruvmanila:github-action

Conversation

@dhruvmanila

@dhruvmaniladhruvmanila commented Sep 30, 2020

Copy link
Copy Markdown
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

dhruvmanila commented Sep 30, 2020•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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

dhruvmanila commented Sep 30, 2020

Copy link
Copy Markdown
MemberAuthor

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

cclauss
cclauss reviewed Sep 30, 2020
View reviewed changes
Comment thread.github/workflows/pre-commit.yml Outdated
cclauss
cclauss requested changes Sep 30, 2020
View reviewed changes

@cclausscclauss left a comment

Copy link
Copy Markdown
Member

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.

Comment thread.github/workflows/pre-commit.yml Outdated
Comment thread.github/workflows/pre-commit.yml Outdated
Comment thread.github/workflows/pre-commit.yml Outdated
Comment thread.github/workflows/pre-commit.yml Outdated
@cclauss

cclauss commented Sep 30, 2020

Copy link
Copy Markdown
Member

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

@cclauss

cclauss commented Sep 30, 2020

Copy link
Copy Markdown
Member

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 <cclauss@me.com>
@dhruvmanila

dhruvmanila commented Sep 30, 2020

Copy link
Copy Markdown
MemberAuthor

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:masterSep 30, 2020
@cclausscclauss 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 <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
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 <cclauss@me.com>
Co-authored-by: Christian Clauss <cclauss@me.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cclausscclausscclauss 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