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

Better GitHub Action workflow for JuliaFormatter #213

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

hyrodium
Copy link
Contributor

@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (28d29dc) 75.20% compared to head (d5edb85) 75.20%.
Report is 1 commits behind head on master.

❗ Current head d5edb85 differs from pull request most recent head cdf9461. Consider uploading reports for the commit cdf9461 to get more accurate results

Files Patch % Lines
src/unbound_args.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #213   +/-   ##
=======================================
  Coverage   75.20%   75.20%           
=======================================
  Files          11       11           
  Lines         750      750           
=======================================
  Hits          564      564           
  Misses        186      186           
Flag Coverage Δ
unittests 75.20% <66.66%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@hyrodium
Copy link
Contributor Author

The original workflow julia-code-style-suggesters is a combination of:

  • JuliaFormatter.jl
  • JuliaProjectFormatter.jl
  • reviewdog

All features in JuliaProjectFormatter.jl seems a subset of Aqua.jl, so there would be no problem in this PR. (tkf/JuliaProjectFormatter.jl#7 (comment))

@lgoettgens lgoettgens added the ci label Oct 17, 2023
@lgoettgens
Copy link
Collaborator

In #218, I tried this action, but didn't get any suggestions, only stuff in the CI log. Do you now, what (github settings, CI permissions, etc.) needs to be changed for this to work for PRs from forks?

@hyrodium
Copy link
Contributor Author

The log shows the folloing message, so it seems we need more permissions for that token.

reviewdog: This GitHub token doesn't have write permission of Review API [1],
so reviewdog will report results via logging command [2] and create annotations similar to
github-pr-check reporter as a fallback.
[1]: https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target,
[2]: https://help.github.com/en/actions/automating-your-workflow-with-github-actions/development-tools-for-github-actions#logging-commands

@lgoettgens lgoettgens mentioned this pull request Oct 24, 2023
@lgoettgens
Copy link
Collaborator

I applied @fingolfin's suggestions from #218 here.

@lgoettgens lgoettgens force-pushed the feature/better_formatter branch from 519455f to 3688186 Compare October 24, 2023 14:00
@lgoettgens
Copy link
Collaborator

reviewdog: environment variable $REVIEWDOG_GITHUB_API_TOKEN is not set https://github.com/JuliaTesting/Aqua.jl/actions/runs/6627926585/job/18003929859?pr=213#step:5:185

@fingolfin something seems to be wrong

@hyrodium
Copy link
Contributor Author

I have applied the commits from #218. (x-ref #218 (comment))

@hyrodium
Copy link
Contributor Author

We still have the following message:

reviewdog: This GitHub token doesn't have write permission of Review API [1],

https://github.com/JuliaTesting/Aqua.jl/actions/runs/6642425658/job/18047160049?pr=213#step:5:192

@hyrodium
Copy link
Contributor Author

hyrodium commented Nov 3, 2023

We can use julia-actions/julia-format@v2 when julia-actions/julia-format#24 is merged.

@lgoettgens lgoettgens force-pushed the feature/better_formatter branch from 80dd482 to a2aee99 Compare December 1, 2023 11:36
@lgoettgens
Copy link
Collaborator

With the julia-format@v2 action, there is still the same error as in #213 (comment).

@lgoettgens lgoettgens force-pushed the feature/better_formatter branch from d5edb85 to cdf9461 Compare December 1, 2023 16:28
@hyrodium
Copy link
Contributor Author

hyrodium commented Dec 2, 2023

Hmm, the suggestion works file in other PRs (#255 (comment)) now, so I'm not sure the reason for that error.
Perhaps, recreating this PR would solve the problem?

@lgoettgens
Copy link
Collaborator

Hmm, the suggestion works file in other PRs (#255 (comment)) now, so I'm not sure the reason for that error. Perhaps, recreating this PR would solve the problem?

The only difference to other PRs that I can see is that this PR here is from a fork.

@hyrodium
Copy link
Contributor Author

hyrodium commented Dec 2, 2023

The only difference to other PRs that I can see is that this PR here is from a fork.

That's correct. I found an issue about this: reviewdog/action-suggester#19

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

Successfully merging this pull request may close these issues.

3 participants