Add reporter for outputing Reviewdog JSONL format #377

ewencluley · 2021-03-28T15:07:27Z

This reporter will convert linter output to a standardised format that can
easily be read by reviewdog.

I have decided to use the JSONL format over the JSON format as it will be
simpler to join together mutltiple files before feeding into review dog.

Very much a WIP and would like feedback on whether this approach is sensible or not.

Currently I've done a converter for Flake8 and Black but would obviously need to do many more, though as I work on this some commonalities will emerge e.g. those linters that output unified diffs can likely share a converter.

TODO:

RDJSON output format for eslint + any other typescript/javascript linters
Documentation

nvuillam · 2021-03-28T21:34:32Z

Great, i'll have a look :)

nvuillam · 2021-03-28T21:39:39Z

Seems promising !
Do you think that we could make generic stdout/err post-processing methods for all linters, and we could define in descriptor YML files the method to call for reviewdog post-processing ?

I'd like to avoid as much as possible to have direct references to linters in core Mega-Linter architecture, and it would be easier to maintain than having a method for each linter ^^

ewencluley · 2021-03-29T13:42:30Z

Seems promising !
Do you think that we could make generic stdout/err post-processing methods for all linters, and we could define in descriptor YML files the method to call for reviewdog post-processing ?

I'd like to avoid as much as possible to have direct references to linters in core Mega-Linter architecture, and it would be easier to maintain than having a method for each linter ^^

That sounds sensible- I can look into doing that, yeah. I think the 2 convertors I have made so far will be pretty re-usable as one is for unified diff and the other is a pretty simple pattern matching sort of thing.

ewencluley · 2021-03-29T20:21:29Z

To clarify, before I go down some 🐰 🕳️, do you mean adding a post_process method to the Linter base class that would be called from the ReviewdogJsonlReporter and would use the linter description to determine which specific processor/convertor to call (e.g. UnifiedDiffRdjsonlConvertor)?

nvuillam · 2021-03-31T21:03:31Z

Sorry for the delay... lot of work on the job that pays my rent ^^
Do you think we can call reviewdog after each linter, or in "bulk" mode at the end ?

We can have linter reporters and mega-linter reporter, so for example we could collect/transform all data after each linter, then send the whole package at the end

And for me reviewdog reporter could itself receive "reviewdog_processor" from .yml descriptors (it can become a standard yml attribute, reviewdog looks so great that it deserves it ^^ )

ewencluley · 2021-04-02T14:31:02Z

Sorry for the delay... lot of work on the job that pays my rent ^^

Same for me. Don't worry about slow replies, great to have this level of engagement from a project maintainer 😄

I have been primarily focused on looking at reviewdog from a github perspective (will have to investigate bitbucket and gitlab at some point).

I think there are pros and cons to each approach.

If we call it once for the overal result of all linters (this was what I was thinking would be the best approach) we will get a single review on the PR (using the github-pr-review reporter in reviewdog) for all linters together. RD automatically limits the number of line comments it posts with the review to 30 to avoid hitting githubs rate limit(see here ). It will add any over the limit to the summary comment for the review so no output is lost.

If it is called once for each linter output that would generate a seperate "review" for each linter with the 30 comment limit per linter output, however the rate limit could still kick in if we are calling RD multiple times in quick sucession. This way we would have less control over whether it hit the rate limit or not.

I think calling RD once for all the linters togehter is the best approach and this can easily be achieved by joinign together the RDJSONL output as the input to RD. I also think this will allow us to just let RD do it's thing with any of its reporters.

ewencluley · 2021-04-02T16:33:00Z

Have updated this to allow the reviewdog processor to be defined in the descriptor file for each linter. I'm not quite sure ive got the json schema stuff right but it seems to ./build.sh ok. If not defined, the reporter will currently just skip writing output files (temporary behaviour). Let me know if this is along the lines of what you were thinking @nvuillam.

nvuillam · 2021-04-02T16:58:38Z

That is exactly what i was thinking :)

ewencluley · 2021-04-04T11:48:59Z

@nvuillam 2 questions:

The reviewdog_processor property I have added to the descriptor is not getting picked up when the linter runs on this PR. Do you know why that would be happening? Is it expected? When i run locally it is working fine and picking up the property. (running locally using docker build -f flavors/python/Dockerfile . -t mega-linter && docker run -it -e ENABLE=PYTHON -e ADDITIONAL_EXCLUDED_DIRECTORIES=venv -e REVIEWDOG_REPORTER=true -v "/Users/ewen/example-project:/tmp/lint" mega-linter)
How am I best to add the install command for reviewdog to the docker file? I can see the build.sh that uses the linter descriptors to determin what needs installed but is there a place to define installation commands for all dockerfile flavours? I dont think I can just add it to the Dockerfile as it will be overwriten?

nvuillam · 2021-04-04T18:27:41Z

MegaLinter job on a PR uses the previous version of MegaLinter, so it's ok that this job fails on case of a major evolution impacting new YML properties
While the big PR job pass, it's ok :) ( it contains a step linting own repo with just built MegaLinter image
If you set Quick build in the commit message, it can reuse previously built image so the PR job is way faster. But to have access to QuickBuild function you need to be an internal contributor of the main repo, I give you access :)
you can put it here :)

ewencluley · 2021-04-04T19:29:42Z

Awesome, thanks. I have added 2 more commits, one to install reviewdog and one to post to the PR. The later I ahve not been able to test completely as locally I am not working on a PR and in thsi PR (as identified above) it doesnt run with the latest changes in this PR so the descriptors dont have the config to make the the review dog per linter reporter run.

codecov-io · 2021-04-05T09:23:41Z

Codecov Report

Merging #377 (0c80cae) into master (24f2c9c) will increase coverage by 0.85%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
+ Coverage   86.68%   87.54%   +0.85%     
==========================================
  Files         123      125       +2     
  Lines        2915     3082     +167     
==========================================
+ Hits         2527     2698     +171     
+ Misses        388      384       -4

Impacted Files	Coverage Δ
megalinter/setup.py	`0.00% <ø> (ø)`
megalinter/reporters/ReviewdogReporter.py	`58.69% <58.69%> (ø)`
megalinter/reporters/ReviewdogLinterReporter.py	`95.86% <95.86%> (ø)`
megalinter/Reporter.py	`90.47% <100.00%> (ø)`
megalinter/MegaLinter.py	`90.62% <0.00%> (+2.08%)`	⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py	`99.12% <0.00%> (+9.64%)`	⬆️

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 24f2c9c...0c80cae. Read the comment docs.

nvuillam · 2021-04-05T11:38:25Z

Can you add a test case using reviewdog ? This way you'll be able to see if it works from ci :)

ewencluley · 2021-05-17T08:17:28Z

I think that once eslint is managed, we may consider publishing a first version ?
Python Java & JS/TS being the most used, I think it would be enough for a start ^^

Great. That sounds like a good plan. I will work on that and some docs.

I also think it would be sensible to have the reporter disabled by default (currently I have it enabled by default) so users have to explicitly enable it. Happy to defer to your opinion on that though.

nvuillam · 2021-05-17T09:58:05Z

Just a suggestion (I don't know the topic so it may be dumb), but couldn't it possible to use https://github.com/reviewdog/errorformat ?

Do you mean instead of the RDJSON format? In theory it could be, the one issue there is that errorformat AFAICT does not support suggestions, where as RDJSON does. I wouldn't normally care too much but on some of the linters that output a unified diff there is very little detail of the actual error and the diff provides a suggestion.

For example, Black outputs a generic message for all files regardless of the rule violation with the diff, as follows:
❌ [ERROR] radiopi.py
    would reformat radiopi.py
    Oh no! 💥 💔 💥
    1 file would be reformatted.
    --- radiopi.py      2021-04-30 09:49:25.071383 +0000
    +++ radiopi.py      2021-05-17 08:06:48.931037 +0000
   
                alarm = Clock.get_alarm()
    -        alarm_str = f'{str(alarm.hour).zfill(2)}:{str(alarm.minute).zfill(2)} [{Clock.get_days_str(alarm.day_of_week)}]'
    +        alarm_str = f"{str(alarm.hour).zfill(2)}:{str(alarm.minute).zfill(2)} [{Clock.get_days_str(alarm.day_of_week)}]"
     
             display.draw_text((20, 0), time_str, font=display.big_font)
From this we cannot infer the actual issue Black has found, only what it suggests the code is changed too, hence the reliance on suggestions.

It is a real shame there is no industry standard output for linters as that would make this whole thing so much easier.

I don't know the details, I just read in reviewdog that it uses that internally, so maybe it would prevent you from describing the format, as it may already be done by an existing tool ^^
But if it does not fit the requirement, I trust your judgement :)

I think that once eslint is managed, we may consider publishing a first version ?
Python Java & JS/TS being the most used, I think it would be enough for a start ^^

Great. That sounds like a good plan. I will work on that and some docs.

I also think it would be sensible to have the reporter disabled by default (currently I have it enabled by default) so users have to explicitly enable it. Happy to defer to your opinion on that though.

Agreed, Mega-Linter claims that there is no calls to external apps by default, let's respect that :)
But why not a suggestion message in the logs to propose to activate reviewdog ? ( message that could be disabled with another variable, like with FLAVORS_SUGGESTION )

eventually add reviewdog option in the assisted installer ?

nvuillam · 2021-05-17T10:01:04Z

Note: as you are declared as collaborator on the repo, you can create an branch named alpha in the main repo, that would generate automatically an alpha version of Mega-Linter, that could be tested on other repos to be sure it works as expected in a real context :)

nvuillam · 2021-05-24T22:35:27Z

@ewencluley you should install https://www.npmjs.com/package/markdown-table-formatter :) (it is used in build.sh)
npm i markdown-table-formatter -g

nvuillam · 2021-05-24T22:36:57Z

you can also write quick-build in the commit message when u have just updated python code :) ( faster build , does not performs all the installations of Dockerfile )

ewencluley · 2021-05-25T16:52:40Z

Thanks for the pointers. I was trying to test out the alpha branch I created on this PR , but am unsure why it seems to be not getting the alpha version and instead getting v4

https://github.com/ewencluley/radiopi/pull/4/checks?check_run_id=2667467176

nvuillam · 2021-05-25T22:00:59Z

My bad sorry ^^
Set alpha instead of v4 in action.yml and it will be ok :)

ewencluley · 2021-05-27T19:35:30Z

Ah, perfect. Next question- i need to add unidiff as a dependency. I've added it to .automation/setup.py and requirements.dev.txt but is there anywhere else I need to add it. I can see an error in one of the CI logs that is

--Error detail:
************* Module megalinter.reporters.ReviewdogLinterReporter
megalinter/reporters/ReviewdogLinterReporter.py:139:12: E0401: Unable to import 'unidiff' (import-error)

https://github.com/nvuillam/mega-linter/runs/2688240026?check_suite_focus=true

ewencluley · 2021-05-27T19:39:30Z

Sorry, not .automation/setup.py, my mistake, I have it already in:
megalinter/setup.py and requirements.dev.txt

see https://github.com/nvuillam/mega-linter/blob/10821dcac67367d53af15837986de1e18481238d/megalinter/setup.py

ewencluley · 2021-06-25T10:04:06Z

@nvuillam I have just pushed to a new branch (alpha-reviewdog-reporter) and getting the same error

--Error detail:
************* Module megalinter.reporters.ReviewdogLinterReporter
megalinter/reporters/ReviewdogLinterReporter.py:139:12: E0401: Unable to import 'unidiff' (import-error)

https://github.com/nvuillam/mega-linter/runs/2912917400?check_suite_focus=true

I have unidiff in megalinter/setup.py and when i build the docker image locally and run all works fine. Any ideas appreciated. Thanks

P.S. Apologies for the delays in getting this moving again, been on vacation for 3 weeks and just back.

nvuillam · 2021-06-26T22:24:30Z

@ewencluley i'll try to checkout your branch :)

nvuillam

Plz check comments :)

nvuillam · 2021-06-26T22:41:47Z

megalinter/reporters/ReviewdogReporter.py

+        repository_owner, repository = config.get("GITHUB_REPOSITORY").split("/")
+
+        process = subprocess.Popen(
+            "reviewdog -f=rdjsonl -reporter=github-pr-review -filter-mode=nofilter",


Could the reporter have a default value that would be calculated according to ENV variables ? ( github, gitlab, azure ... )

Gitlab: GITLAB_CI
Github action: GITHUB_ACTION
Azure: SYSTEM_TEAMFOUNDATIONCOLLECTIONURI
Travis CI: TRAVIS
Circle CI: CIRCLECI
(cf https://github.com/npm/ci-detect/blob/master/index.js )

I think it would also be nice to be able to add more parameter to reviewdog, like we do for linters, using some var REVIEWDOG_EXTRA_ARGS

nvuillam · 2021-06-26T22:58:55Z

About https://github.com/nvuillam/mega-linter/runs/2912917400?check_suite_focus=true, Mega-Linter's own Mega-Linting during PR uses a merge between latest docker image and updated local code.
So latest image does not contain unidiff, and local code does ,that's what explains the crash

So basically....

focus on Build & Deploy - DEV / Tests + Deploy Docker Image - DEV job, it contains an own MegaLinter linting with just created docker image, so if this one passes, I'll be able to ignore MegaLinter failed job :)

nvuillam · 2021-06-26T23:55:45Z

You also need to fix real MegaLinter errors

Copy-Paste:

Clone found (python):
 - megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py [52:76 - 65:6] (13 lines, 89 tokens)
   megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py [28:55 - 41:7]

CSpell:
Copy .cspell.json in your local branch and check the git diff to see if there are real typos or not (https://github.com/nvuillam/mega-linter/suites/2817971503/artifacts/62734442)

Python:

/tmp/lint/megalinter/reporters/ReviewdogLinterReporter.py:67:121: E501 line too long (126 > 120 characters)
/tmp/lint/megalinter/reporters/ReviewdogLinterReporter.py:75:17: F841 local variable 'file_nm' is assigned to but never used
/tmp/lint/megalinter/reporters/ReviewdogLinterReporter.py:161:27: E741 ambiguous variable name 'l'
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:44:121: E501 line too long (211 > 120 characters)
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:45:121: E501 line too long (211 > 120 characters)
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:79:121: E501 line too long (211 > 120 characters)
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:80:121: E501 line too long (211 > 120 characters)
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:109:1: W293 blank line contains whitespace
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:116:1: W293 blank line contains whitespace
/tmp/lint/megalinter/tests/test_megalinter/reporters/ReviewDogLinterReporterTest.py:130:121: E501 line too long (754 > 120 characters)

ewencluley · 2021-07-10T07:52:04Z

@nvuillam - I've fixed the linting errors but the build on the alpha-reviewdog-reporter branch seems to timeout now after 1 hour. I have no idea what is causing it to do so and no idea why it is taking this length of time.
https://github.com/nvuillam/mega-linter/runs/3031852908?check_suite_focus=true

nvuillam · 2021-07-10T07:59:40Z

@nvuillam - I've fixed the linting errors but the build on the alpha-reviewdog-reporter branch seems to timeout now after 1 hour. I have no idea what is causing it to do so and no idea why it is taking this length of time.
https://github.com/nvuillam/mega-linter/runs/3031852908?check_suite_focus=true

Please can you try to increase timeout properties in CI job config ?
https://github.com/nvuillam/mega-linter/blob/master/.github/workflows/deploy-DEV.yml

ewencluley · 2021-07-10T11:10:50Z

Please can you try to increase timeout properties in CI job config ?
https://github.com/nvuillam/mega-linter/blob/master/.github/workflows/deploy-DEV.yml

Awesome, that seems to have worked. I am trying to test it out on this PR https://github.com/ewencluley/radiopi/actions/runs/1017820202

Having some trouble working out what the uses should be in the workflow yaml.
https://github.com/ewencluley/radiopi/pull/4/files#diff-206ce5348d308a71dab8cdc132d6dc2babebc80b94a707a45a744909ed9367d4R44

Have tried:
uses: nvuillam/mega-linter@test-ewencluley-alpha-reviewdog-reporter - the tag of the docker image on docker hub
This gives me

Unable to resolve action nvuillam/mega-linter@test-ewencluley-alpha-reviewdog-reporter

and
uses: nvuillam/mega-linter@alpha-reviewdog-reporter - the branch name
That gives me

Pull down action image 'nvuillam/mega-linter:alpha-reviewdog-reporter'
  /usr/bin/docker pull nvuillam/mega-linter:alpha-reviewdog-reporter
  Error response from daemon: manifest for nvuillam/mega-linter:alpha-reviewdog-reporter not found: manifest unknown: manifest unknown

Can you advise @nvuillam?

nvuillam · 2021-07-10T11:26:08Z

You need to be in alpha branch and use the alpha version built by deploy docker image - alpha job

alpha must be the exact name ^^

ewencluley · 2021-07-10T11:26:48Z

You need to be in alpha branch and use the alpha version built by deploy docker image - alpha job

alpha must be the exact name ^^

Oh! Makes sense. Ok for me to push it to the alpha branch?

nvuillam · 2021-07-10T13:49:08Z

alpha branch is yours :)

ewencluley · 2021-07-10T21:38:21Z

Victory!!!! I have finally got it to post comments on a PR!!

ewencluley/radiopi#4

This reporter will convert linter output to a standardised format that can easily be read by reviewdog. I have decided to use the JSONL format over the JSON format as it will be simpler to join together mutltiple files before feeding into review dog. Install reviewdog To use with the reviewdog reporter we install reviewdog in the docker image. This intentionally pinned to a specific version to avoid issues if breaking changes occur in Reviewdog. Add overall reviewdog reporter This gathers results generated by the Reviewdog Linter Reporters and feeds those results into reviewdog. This will make reviewdog post comments to github as a PR review. Configure pylint for reviewdog reporter add unit tests for reviewdog reporter Reviewdog: Checkstyle config Dont run reviewdog reporter by default

codecov-commenter · 2021-07-12T07:47:56Z

Codecov Report

Merging #377 (80decf8) into master (5cab12d) will decrease coverage by 1.74%.
The diff coverage is 36.84%.

@@            Coverage Diff             @@
##           master     #377      +/-   ##
==========================================
- Coverage   86.67%   84.92%   -1.75%     
==========================================
  Files         130      132       +2     
  Lines        2986     3157     +171     
==========================================
+ Hits         2588     2681      +93     
- Misses        398      476      +78

Impacted Files	Coverage Δ
megalinter/setup.py	`0.00% <ø> (ø)`
megalinter/reporters/ReviewdogLinterReporter.py	`32.78% <32.78%> (ø)`
megalinter/reporters/ReviewdogReporter.py	`46.93% <46.93%> (ø)`
megalinter/MegaLinter.py	`90.68% <0.00%> (+2.41%)`	⬆️
megalinter/reporters/UpdatedSourcesReporter.py	`89.18% <0.00%> (+2.70%)`	⬆️
...alinter/tests/test_megalinter/helpers/utilstest.py	`97.05% <0.00%> (+9.24%)`	⬆️

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 5cab12d...80decf8. Read the comment docs.

github-actions · 2021-08-12T00:35:09Z

This pull request has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this pull request should stay open, please remove the O: stale 🤖 label or comment on the pull request.

ewencluley force-pushed the reviewdog-reporter branch from 92ee288 to 553fc49 Compare March 28, 2021 15:09

ewencluley mentioned this pull request Mar 28, 2021

ReviewDog reporter for Mega-Linter reviewdog/reviewdog#841

Open

ewencluley force-pushed the reviewdog-reporter branch 2 times, most recently from b86e034 to 4b963cc Compare April 2, 2021 16:30

ewencluley force-pushed the reviewdog-reporter branch 4 times, most recently from 6d46fde to 936140e Compare April 4, 2021 11:25

ewencluley force-pushed the reviewdog-reporter branch 2 times, most recently from 1d9e6fc to 0846dd0 Compare April 4, 2021 19:26

ewencluley force-pushed the reviewdog-reporter branch from 0846dd0 to b2852ec Compare April 5, 2021 08:45

ewencluley force-pushed the reviewdog-reporter branch 5 times, most recently from 5a74495 to 8fedc6e Compare April 8, 2021 20:52

ewencluley force-pushed the reviewdog-reporter branch from aa82bc7 to 04ce942 Compare April 17, 2021 08:44

nvuillam requested changes Jun 26, 2021

View reviewed changes

nvuillam mentioned this pull request Jul 3, 2021

Support for GitLab "Code Quality" artifacts #539

Closed

ewencluley force-pushed the reviewdog-reporter branch from d140b4e to 60f82cf Compare July 12, 2021 07:00

ewencluley and others added 2 commits July 12, 2021 08:05

add reviewdog for javascript linters

80decf8

ewencluley force-pushed the reviewdog-reporter branch from 60f82cf to 80decf8 Compare July 12, 2021 07:06

github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Aug 12, 2021

github-actions bot closed this Aug 27, 2021

Add reporter for outputing Reviewdog JSONL format #377

Add reporter for outputing Reviewdog JSONL format #377

Conversation

ewencluley commented Mar 28, 2021 • edited

nvuillam commented Mar 28, 2021

nvuillam commented Mar 28, 2021

ewencluley commented Mar 29, 2021

ewencluley commented Mar 29, 2021

nvuillam commented Mar 31, 2021 • edited

ewencluley commented Apr 2, 2021

ewencluley commented Apr 2, 2021

nvuillam commented Apr 2, 2021 • edited

ewencluley commented Apr 4, 2021 • edited

nvuillam commented Apr 4, 2021

ewencluley commented Apr 4, 2021

codecov-io commented Apr 5, 2021 • edited

Codecov Report

nvuillam commented Apr 5, 2021

ewencluley commented May 17, 2021

nvuillam commented May 17, 2021 • edited

nvuillam commented May 17, 2021

nvuillam commented May 24, 2021

nvuillam commented May 24, 2021

ewencluley commented May 25, 2021 • edited

nvuillam commented May 25, 2021

ewencluley commented May 27, 2021

ewencluley commented May 27, 2021 • edited

ewencluley commented Jun 25, 2021

nvuillam commented Jun 26, 2021

nvuillam left a comment

Choose a reason for hiding this comment

nvuillam Jun 26, 2021

Choose a reason for hiding this comment

nvuillam Jun 26, 2021

Choose a reason for hiding this comment

nvuillam Jun 26, 2021

Choose a reason for hiding this comment

nvuillam commented Jun 26, 2021 • edited

nvuillam commented Jun 26, 2021

ewencluley commented Jul 10, 2021

nvuillam commented Jul 10, 2021

ewencluley commented Jul 10, 2021

nvuillam commented Jul 10, 2021

ewencluley commented Jul 10, 2021

nvuillam commented Jul 10, 2021

ewencluley commented Jul 10, 2021

codecov-commenter commented Jul 12, 2021

Codecov Report

github-actions bot commented Aug 12, 2021

ewencluley commented Mar 28, 2021 •

edited

nvuillam commented Mar 31, 2021 •

edited

nvuillam commented Apr 2, 2021 •

edited

ewencluley commented Apr 4, 2021 •

edited

codecov-io commented Apr 5, 2021 •

edited

nvuillam commented May 17, 2021 •

edited

ewencluley commented May 25, 2021 •

edited

ewencluley commented May 27, 2021 •

edited

nvuillam commented Jun 26, 2021 •

edited