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 support for GitHub annotations in GitHub actions #1052

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

nijel
Copy link

@nijel nijel commented Jul 19, 2024

  • Added a change log entry in changelog.d/<directory>/.
  • Added self to copyright blurb of touched files.
  • Added self to AUTHORS.rst.
  • Wrote tests.
  • Documented my changes in docs/man/ or elsewhere.
  • My changes do not contradict
    the current specification.
  • I agree to license my contribution under the licenses indicated in the
    changed files.

nijel added 3 commits July 19, 2024 14:40
This allows to add more formaters based on the same error list.
This allows GitHub to parse it and display annotations on pull requests.
Use REUSE_OUTPUT_FORMAT to allow overriding lint output formats in certain
environments (like CI).
nijel added a commit to WeblateOrg/meta that referenced this pull request Jul 19, 2024
Hopefully this will land via fsfe/reuse-tool#1052.
@carmenbianca
Copy link
Member

Wow, amazing PR @nijel ! Thank you for this work.

I wonder whether it makes sense to keep --lines in parallel to --github. They do effectively the same. For backward compatibility, we should probably keep both, though…

Do you know whether this GitHub format of errors is also consumed by other programs?

@nijel
Copy link
Author

nijel commented Jul 19, 2024

--lines is still more user friendly, so I'd keep both. I think gitlab has a similar format, but I have no experience with that. https://docs.astral.sh/ruff/settings/#output-format supports that in addition to GitHub...

Copy link
Member

@carmenbianca carmenbianca left a comment

Choose a reason for hiding this comment

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

Some questions and feedback. The PR looks great overall.

I will write a few patch commits to fix the easy stuff that doesn't need discussion.

src/reuse/lint.py Outdated Show resolved Hide resolved
src/reuse/lint.py Outdated Show resolved Hide resolved
src/reuse/lint.py Outdated Show resolved Hide resolved
Comment on lines +107 to +114
Environment
-----------

.. envvar:: REUSE_OUTPUT_FORMAT

Specifies output format, one of ``plain``, ``lines``, ``github``, ``json``

It behaves same as corresponding command line options.
Copy link
Member

Choose a reason for hiding this comment

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

There should be a documented order of precedence here. If both REUSE_OUTPUT_FORMAT and a CLI flag are used, which takes precedence?

As written in the code, I believe the env var takes precedence. I do not know if this is desirable behaviour.

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, should quiet be a valid value here?

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, what should happen if the value of REUSE_OUTPUT_FORMAT is invalid?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, seems like I didn't notice this feedback, sorry.

With the current implementation, the environment variable takes precedence. This is intentional to allow having command line configuration and override it by environment if executed in environment which might want different output (as GitHub).

Unknown values are simply discarded, but it might be a better approach to issue a warning.

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
I added a rather silly fallback. Maybe an error should be raised, but I
don't want to deal with catching this very obscure error.

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
The first implementation of this consumed the `reuse lint --json`
output. It was later patched up to consume the ProjectReport instead,
but the docstrings were apparently never adjusted accordingly.

Signed-off-by: Carmen Bianca BAKKER <carmenbianca@fsfe.org>
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.

2 participants