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 base_sha option to clj-lint-action. #198

Merged
merged 1 commit into from
Jun 3, 2020

Conversation

niyarin
Copy link
Contributor

@niyarin niyarin commented May 29, 2020

This PR adds the option of clj-lint-action.
Without this option, the second commit on the branch would not be able to take the diff.

@niyarin niyarin requested review from alumi and a team as code owners May 29, 2020 05:44
@niyarin niyarin requested review from yito88 and removed request for a team May 29, 2020 05:44
@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #198 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #198   +/-   ##
=======================================
  Coverage   86.90%   86.90%           
=======================================
  Files          77       77           
  Lines        6246     6246           
  Branches      519      519           
=======================================
  Hits         5428     5428           
  Misses        299      299           
  Partials      519      519           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806aad3...2cbda5a. Read the comment docs.

@@ -12,4 +12,5 @@ jobs:
linters: "[\"clj-kondo\" \"kibit\" \"eastwood\"]"
github_token: ${{ secrets.GITHUB_TOKEN }}
runner: ":leiningen"
base_sha: ${{ github.event.before }}
Copy link
Member

@alumi alumi May 29, 2020

Choose a reason for hiding this comment

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

I'm sorry if I misunderstood, but I guess github.event.pull_request.base.sha is more suitable for PR events since files eventually changed in a PR are more important than files changed in the last commit of a PR.

@niyarin niyarin force-pushed the fix/add-clj-lint-action-option branch from 0444810 to 2039561 Compare May 29, 2020 08:33
@niyarin
Copy link
Contributor Author

niyarin commented May 29, 2020

Thank you for the pointing.
I changed the options for pull requests as well as push.

@alumi
Copy link
Member

alumi commented May 29, 2020

Hmm..
In https://github.com/chrovis/cljam/pull/198/checks?check_run_id=719698533, the github.event.before on a push event seems to refer to 0444810 which is a force-pushed obsolete commit 🤔

@niyarin niyarin force-pushed the fix/add-clj-lint-action-option branch from 2039561 to 2cbda5a Compare June 1, 2020 06:48
@niyarin
Copy link
Contributor Author

niyarin commented Jun 1, 2020

For a non-master branch, I changed the action to pick up the changes from the beginning of the branch.
・Changed clj-lint-action to take a diff from the head of that branch when passing an empty commit sha.
・Changed lint.yml to take base_sha only for master.

Copy link
Member

@alumi alumi 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 for your quick response!

Copy link
Contributor

@yito88 yito88 left a comment

Choose a reason for hiding this comment

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

LGTM!
Thank you 👍

@alumi alumi merged commit de095a1 into master Jun 3, 2020
@alumi alumi deleted the fix/add-clj-lint-action-option branch June 3, 2020 01:19
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.

3 participants