Add new JSON reporter #8929

DanielNoord · 2023-08-05T16:56:45Z

Type of Changes

	Type
✓	✨ New feature

Description

Closes #4741
Closes #3504

As discussed in the issue we create a new json2 reporter that users can now use to get the score and other statistics. Since I want to keep being conscious about what is and isn't public API I decided to put both reporters in the private namespace. Since we are still in beta that should be fine? 😄

DanielNoord · 2023-08-05T17:00:18Z

@jacobtylerwalls Is the issue on PyPy related to changes to how we invoke tests? The module is clearly there 😅

Pierre-Sassoulas

Some preliminary comments, I'll do a more thorough review later (I'm on mobilel) I had only a quick look but nothing seems to be wrong

doc/whatsnew/fragments/4741.feature

Pierre-Sassoulas · 2023-08-05T17:13:18Z

pylint/lint/base_options.py

@@ -103,7 +103,7 @@ def _make_linter_options(linter: PyLinter) -> Options:
                "short": "f",
                "group": "Reports",
                "help": "Set the output format. Available formats are text,"
-                " parseable, colorized, json and msvs (visual studio)."
+                " parseable, colorized, json2 and msvs (visual studio)."


I don't think we should hide the fact that the json reporter still exists, generating this text would be nice too imo.

I guess we have a different opinion here then.

I think there should be one way to do things and that json2 is superior. Thus, that option is what we should advertise to users looking through the possibilities.

Yes, but imagine you maintain a tool that consumes the old reporter. And you're reviewing a PR, and you need to brush up on the context. You come here, and it's gone. Won't you burn time trying to figure out what happened and wondering why things aren't horribly broken?

Isn't that what the changelog is for? If you search for json you'll find the change.

I'm fine with adding something like json (deprecated) but as expressed elsewhere I would really like to nudge all users to the new reporter.

Yeah I'm waffling on this one. As long as it's somewhere I guess it doesn't have to be in the CLI help.

jacobtylerwalls · 2023-08-05T17:50:25Z

@jacobtylerwalls Is the issue on PyPy related to changes to how we invoke tests?

I don't know, I'd appreciate help investigating.

The module is clearly there 😅

From recent experience, no, nothing about package layouts without src-layout is clear ...! 😆

jacobtylerwalls

Big thanks for picking this up!

doc/whatsnew/fragments/4741.feature

jacobtylerwalls · 2023-08-05T21:16:01Z

pylint/lint/base_options.py

@@ -103,7 +103,7 @@ def _make_linter_options(linter: PyLinter) -> Options:
                "short": "f",
                "group": "Reports",
                "help": "Set the output format. Available formats are text,"
-                " parseable, colorized, json and msvs (visual studio)."
+                " parseable, colorized, json2 and msvs (visual studio)."


Yes, but imagine you maintain a tool that consumes the old reporter. And you're reviewing a PR, and you need to brush up on the context. You come here, and it's gone. Won't you burn time trying to figure out what happened and wondering why things aren't horribly broken?

pylint/reporters/_json_reporter.py

pylint/testutils/_primer/primer_command.py

tests/reporters/unittest_reporting.py

jacobtylerwalls · 2023-08-05T21:55:21Z

Since I want to keep being conscious about what is and isn't public API I decided to put both reporters in the private namespace. Since we are still in beta that should be fine? 😄

My initial inclination is that we need a stronger reason to do this, but I'm not certain yet. Is there a reason we need to be this paranoid? Don't we anticipate never touching this again?

DanielNoord · 2023-08-05T22:10:22Z

Since I want to keep being conscious about what is and isn't public API I decided to put both reporters in the private namespace. Since we are still in beta that should be fine? 😄

My initial inclination is that we need a stronger reason to do this, but I'm not certain yet. Is there a reason we need to be this paranoid? Don't we anticipate never touching this again?

After seeing the trouble CPython is having with their C API and our own troubles with deprecating stuff I'm (currently) inclined to always make things private unless explicitly asked for. There is a good reason pytest has almost all of their package in a private namespace:
https://github.com/pytest-dev/pytest/tree/main/src/pytest

Whenever somebody want to access something I'm highly in favour of making it public and creating a well-defined API for it, but I just don't want to deal with another spree of deprecation and removal issues.

cdce8p · 2023-08-05T23:58:54Z

My initial inclination is that we need a stronger reason to do this, but I'm not certain yet. Is there a reason we need to be this paranoid? Don't we anticipate never touching this again?

After seeing the trouble CPython is having with their C API and our own troubles with deprecating stuff I'm (currently) inclined to always make things private unless explicitly asked for. There is a good reason pytest has almost all of their package in a private namespace: https://github.com/pytest-dev/pytest/tree/main/src/pytest

Whenever somebody want to access something I'm highly in favour of making it public and creating a well-defined API for it, but I just don't want to deal with another spree of deprecation and removal issues.

It it really necessary to add an underscore? Yes deprecations are quite a hassle, but it also has it's upsides. E.g. if users want to customize any of the existing reporters, they can easily subclass it. As long as it isn't documented publicly, it's just kindness / good responsibility of us not to break stuff unnecessarily.

Another example, consider the checkers/utils.py module which is probably used by a lot of plugins. We can still change things. Plugins authors will adjust. Only it shouldn't be for x.1, then x.2, x.3 and so on. Change it once and stick with it (at least for some time).

DanielNoord · 2023-08-06T07:46:18Z

E.g. if users want to customize any of the existing reporters, they can easily subclass it. As long as it isn't documented publicly, it's just kindness / good responsibility of us not to break stuff unnecessarily.

If there is a need for it they can for it to become public and we can then make that explicit. For me using the underscores is also about setting expectations about what can and what can not change. I'm fine with maintaining a large public API, but only when we see a need for it because plugin writers or users ask for it. From the current state we are in we can only get there by making new code explicitly private.

Another example, consider the checkers/utils.py module which is probably used by a lot of plugins. We can still change things. Plugins authors will adjust. Only it shouldn't be for x.1, then x.2, x.3 and so on. Change it once and stick with it (at least for some time).

Again, I think there is too much of a grey array there. There were quite a bit of number of functions that I would have liked to change or delete when doing the configuration rewrite and the typing stuff but which I couldn't. Some utils should just be for internal use and should be documented as such.

codecov · 2023-08-06T08:52:27Z

Codecov Report

Merging #8929 (a9002af) into main (f40e9ff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8929   +/-   ##
=======================================
  Coverage   95.74%   95.75%           
=======================================
  Files         173      173           
  Lines       18558    18592   +34     
=======================================
+ Hits        17768    17802   +34     
  Misses        790      790

Files Changed	Coverage Δ
pylint/lint/base_options.py	`100.00% <ø> (ø)`
pylint/interfaces.py	`100.00% <100.00%> (ø)`
pylint/reporters/__init__.py	`100.00% <100.00%> (ø)`
pylint/reporters/json_reporter.py	`100.00% <100.00%> (ø)`
pylint/testutils/_primer/primer_run_command.py	`30.76% <100.00%> (-1.05%)`	⬇️

doc/whatsnew/fragments/4741.feature

Pierre-Sassoulas · 2023-08-08T12:18:31Z

pylint/interfaces.py

@@ -35,3 +35,4 @@ class Confidence(NamedTuple):

 CONFIDENCE_LEVELS = [HIGH, CONTROL_FLOW, INFERENCE, INFERENCE_FAILURE, UNDEFINED]
 CONFIDENCE_LEVEL_NAMES = [i.name for i in CONFIDENCE_LEVELS]
+CONFIDENCE_MAP = {i.name: i for i in CONFIDENCE_LEVELS}


It pains me to see that we're basically recreating the enum.Enum interface for our 5 confidences constants and their 1 helper class and 3 helper constants. I don't think we should refactor but it does itch me every time.

Pierre-Sassoulas · 2023-08-08T12:21:46Z

pylint/reporters/_json_reporter.py

+                evaluation, {}, {**counts_dict, "statement": stats.statement or 1}
+            )
+        except Exception as ex:  # pylint: disable=broad-except
+            score: str | int = f"An exception occurred while rating: {ex}"


We can mock an exception in the score evaluation in order to get coverage, or maybe provide a bad string via config that will create an error ?

tests/reporters/unittest_reporting.py

doc/whatsnew/fragments/4741.feature

jacobtylerwalls

I think I agree with Marc that we should prefer building new reporters like json3 if it ever came to that rather than make this private.

DanielNoord · 2023-08-13T17:49:33Z

I rebased and force pushed to remove the private making commits.

This now only introduce a new reporter and uses it where necessary/possible. Let me know what you guys think!

github-actions · 2023-08-13T18:22:08Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 53a5835

tests/reporters/unittest_json_reporter.py

Pierre-Sassoulas

Look ready, it only needs the example of json in 3.0.md that Jacob suggested and let's merge 👍

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

DanielNoord added the Enhancement ✨ Improvement to a component label Aug 5, 2023

DanielNoord added this to the 3.0.0a7 milestone Aug 5, 2023

DanielNoord requested a review from Pierre-Sassoulas August 5, 2023 16:56

DanielNoord mentioned this pull request Aug 5, 2023

Add a new JsonReporter with all the messages information that will replace the current one in 3.0 #7087

Closed

Pierre-Sassoulas requested changes Aug 5, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

jacobtylerwalls reviewed Aug 5, 2023

View reviewed changes

jacobtylerwalls modified the milestones: 3.0.0a7, 3.0.0b1 Aug 6, 2023

DanielNoord requested review from Pierre-Sassoulas and jacobtylerwalls August 6, 2023 08:32

This comment has been minimized.

Sign in to view

Pierre-Sassoulas requested changes Aug 8, 2023

View reviewed changes

jacobtylerwalls requested changes Aug 13, 2023

View reviewed changes

Add a json2 reporter

53a5835

DanielNoord force-pushed the json-reporter branch from a4fcf2d to 53a5835 Compare August 13, 2023 17:49

DanielNoord requested review from jacobtylerwalls and Pierre-Sassoulas August 13, 2023 17:49

jacobtylerwalls reviewed Aug 13, 2023

View reviewed changes

tests/reporters/unittest_json_reporter.py Outdated Show resolved Hide resolved

Pierre-Sassoulas requested changes Aug 15, 2023

View reviewed changes

Update tests/reporters/unittest_json_reporter.py

21718d6

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>

jacobtylerwalls modified the milestones: 3.0.0b1, 3.0.0a7 Aug 15, 2023

Merge remote-tracking branch 'origin/main' into json-reporter

c0ff1fa

Pierre-Sassoulas mentioned this pull request Aug 15, 2023

Bump pylint to 3.0.0a7, update changelog #8958

Merged

Pierre-Sassoulas added 2 commits August 15, 2023 17:40

Upgrade doc

5557e86

Add json example in release note

a9002af

jacobtylerwalls approved these changes Aug 15, 2023

View reviewed changes

Pierre-Sassoulas enabled auto-merge (squash) August 15, 2023 16:07

Pierre-Sassoulas approved these changes Aug 15, 2023

View reviewed changes

Pierre-Sassoulas merged commit 7264e33 into pylint-dev:main Aug 15, 2023
44 checks passed

DanielNoord deleted the json-reporter branch August 15, 2023 18:19

Splines mentioned this pull request Feb 17, 2024

GitHub actions output format #9443

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new JSON reporter #8929

Add new JSON reporter #8929

DanielNoord commented Aug 5, 2023 •

edited by Pierre-Sassoulas

DanielNoord commented Aug 5, 2023

Pierre-Sassoulas left a comment

Pierre-Sassoulas Aug 5, 2023

DanielNoord Aug 5, 2023

jacobtylerwalls Aug 5, 2023

DanielNoord Aug 5, 2023

jacobtylerwalls Aug 5, 2023

This comment has been minimized.

jacobtylerwalls commented Aug 5, 2023

jacobtylerwalls left a comment

jacobtylerwalls Aug 5, 2023

jacobtylerwalls commented Aug 5, 2023

DanielNoord commented Aug 5, 2023

cdce8p commented Aug 5, 2023

DanielNoord commented Aug 6, 2023

codecov bot commented Aug 6, 2023 •

edited

This comment has been minimized.

Pierre-Sassoulas Aug 8, 2023

Pierre-Sassoulas Aug 8, 2023

jacobtylerwalls left a comment

DanielNoord commented Aug 13, 2023

github-actions bot commented Aug 13, 2023

Pierre-Sassoulas left a comment

Add new JSON reporter #8929

Add new JSON reporter #8929

Conversation

DanielNoord commented Aug 5, 2023 • edited by Pierre-Sassoulas

Type of Changes

Description

DanielNoord commented Aug 5, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

jacobtylerwalls commented Aug 5, 2023

jacobtylerwalls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls commented Aug 5, 2023

DanielNoord commented Aug 5, 2023

cdce8p commented Aug 5, 2023

DanielNoord commented Aug 6, 2023

codecov bot commented Aug 6, 2023 • edited

Codecov Report

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacobtylerwalls left a comment

Choose a reason for hiding this comment

DanielNoord commented Aug 13, 2023

github-actions bot commented Aug 13, 2023

Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

DanielNoord commented Aug 5, 2023 •

edited by Pierre-Sassoulas

codecov bot commented Aug 6, 2023 •

edited