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 Reviewdog Diagnostic Format #1018

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

wsipak
Copy link
Collaborator

@wsipak wsipak commented Oct 18, 2021

This PR adds a new output format that can be selected using --output_format flag of the linter.
The format is described here:
https://github.com/reviewdog/reviewdog/tree/master/proto/rdf
It comes in two versions:
rdjson is a JSON that contains one object for the whole linting process, with all violations and fixes inside.
rdjsonl uses JSON Lines to print separate JSON objects for each violation.
We use the second one (rdjsonl).
The content is the same, but this format is easier to produce considering the current interface of ViolationHandler in Verible.

This is done:
There is a class named RDJsonPrinter which implements ViolationHandler interface with rule violations and fixes if they exist.
The output is interpreted by Reviewdog as expected, which can be seen here:
antmicro/gha-playground#102
Some code related to ViolationHandler has been moved from verilog to common.
It seemed like placing RDJsonPrinter inside verilog wouldn't be a good idea, so I've used the occasion to refactor these things.

This isn't done yet:
The --output_format isn't integrated with the --autofix option correctly.
Currently RDJsonPrinter will always include autofixes in the output, and will be selected only if you don't select any --autofix option.
Selecting --autofix=<patch or some other way to fix> results in producing the standard plain text output,
so If you'd like to check out this feature, please just use --output_format=rdjson.
Also, rdjson is to be renamed to rdjsonl so that it's the same name as in Reviewdog.
The name of the standard format is currently plain, and that's the default value.
I think plain, text, plaintext are worth considering.
However, I don't think this is the ultimate name and any feedback on this topic would be much appreciated.

Copy link
Collaborator

@hzeller hzeller left a comment

Choose a reason for hiding this comment

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

I had a quick peek.
I think I'd prefer if you have the refactoring and the additional things in separate pull requests so that it is easier to review and it also makes sense to in building up things.

A good working model is to start with the large CL that does all the things you want (like this one), then you know which parts can be separated out for quick, independent reviews.

@@ -0,0 +1,37 @@
Position:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget copyright header and probalby a link to the spec we try to implement here
( https://github.com/reviewdog/reviewdog/tree/master/proto/rdf )

@hzeller
Copy link
Collaborator

hzeller commented Oct 28, 2021

What is the state here ? I think a good first and easy-to-review step is to do the refactoring moving things to common as there is essentially not much code-change, just moving files arond and fixing up dependencies.

Then the reviewdog output will be a nice small change on top.

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.

None yet

2 participants