Improve JSON output format with score

[angristan]

Is your feature request related to a problem? Please describe

I want to consume this programmatically, but the format make it difficult since the JSON array will contain different types of objects:

[
    {
        "type": "convention",
        "module": "fib",
        "obj": "",
        "line": 1,
        "column": 0,
        "path": "fib.py",
        "symbol": "missing-module-docstring",
        "message": "Missing module docstring",
        "message-id": "C0114"
    },
    {
        "score": "Your code has been rated at 3.33/10"
    }
]

Describe the solution you'd like

Something like this would be better:

{
    "score": 0.333,
    "violations": [
        {
            "type": "convention",
            "module": "fib",
            "obj": "",
            "line": 1,
            "column": 0,
            "path": "fib.py",
            "symbol": "missing-module-docstring",
            "message": "Missing module docstring",
            "message-id": "C0114"
        }
    ]
}

Additional context

The score in the JSON output was added in #3514 for version 3.0.0a0.

[cdce8p]

@DanielNoord Mentioned this issue to me here. As already noted, it's not really possible to add the score to the existing JSONReporter in a backwards compatible manner. I'm wondering though if there is another way to implement it.

This might not be as clean but it wouldn't break existing tools. What if we add a second JSONReporter class for it? I.e. JSONScoreReporter. That would allow anyone who whats to output the score to still use it while not breaking existing tools.

Although an inconsistency, I would recommend not to change the default for JSONReporter even if --score=y is specified. Instead users should choose the alternative / new reporter option for it. That guaranties backwards compatibility.

For the output I would suggest a slight modification. Having score as an object key allows us to add additional keys for it later on without breaking stuff. (There might be better names for it than current and max.)

{
    "score": {
        "current": 0.333,
        "max": 10
    },
    "messages": [
        {
            "type": "convention",
            "module": "fib",
            "obj": "",
            "line": 1,
            "column": 0,
            "path": "fib.py",
            "symbol": "missing-module-docstring",
            "message": "Missing module docstring",
            "message-id": "C0114"
        }
    ]
}

[DanielNoord]

Although an inconsistency, I would recommend not to change the default for JSONReporter even if --score=y is specified. Instead users should choose the alternative / new reporter option for it. That guaranties backwards compatibility.

I'm still 50/50 on whether I think this is the best way forward. Although backward compatibility would be critical here, I think maintaining two different but very similar reporters increases the difficulty of maintaining the project. Not only is there simply more code, but we also need to take more reporters into account when changing things.

Furthermore, I think it is good to somewhat push tools towards using (I believe) improved reporter output or functionality. Taking the example of end_col for vscode: them not being able to access that information currently, even though we provide it, reflects on us rather than on them. Many users will thinks that pylint is not providing the information they need and will look for alternative solutions. Although it is not in anyway a goal to have a much users as possible, I don't think this situation is desired.

To me it makes sense to do a breaking 3.0 change after the Python 3.6 deprecation which includes 1) new and improved output, 2) revamped configuration (argparse or something different) and 3) improved, programmatically created documentation. We could even announce such a breaking change to allow tool maintainers to provide input on potential changes and ask for help preparing for it.

[cdce8p]

Although backward compatibility would be critical here, I think maintaining two different but very similar reporters increases the difficulty of maintaining the project.

It does increase complexity, but we can live with that. IDEs won't be able to adapt that easily. There is a reason the deprecation period for cpython is usually about 6 to 8 years after an alternative implementation has been introduced. For us the ship has sailed, with --msg-template we can't implement it in a backwards compatible manner, same here for the JSONReporter. What I take away from it is that we should put more focus on backwards compatibility when introducing a new user-facing feature. That hasn't always been the case so far, maybe understandably considering the age of the codebase.

IMO breaking changes should be limited to the absolute minimum required to stay sane. If there is a workable alternative, that should almost always be preferred.

Furthermore, I think it is good to somewhat push tools towards using (I believe) improved reporter output or functionality. Taking the example of end_col for vscode: them not being able to access that information currently, even though we provide it, reflects on us rather than on them.

I don't think that is true. We added those just recently. It's normal for new features to be incomplete in the beginning. If we would only ship them once we are 100% certain about everything, we wouldn't release a new version ever.

new and improved output

The default output should not change otherwise we again start to break stuff unnecessarily.

2. revamped configuration (argparse or something different) and 3) improved, programmatically created documentation. We could even announce such a breaking change to allow tool maintainers to provide input on potential changes and ask for help preparing for it.

For configuration and documentation internals the situation is somewhat different. There I would argue it's at least fair to assume that plugin authors are more easily able to update their code. Although even then, we should try to limit them to what's absolutely necessary.

[Pierre-Sassoulas]

We could even announce such a breaking change to allow tool maintainers to provide input on potential changes and ask for help preparing for it.

I announced the 3.0 in pylint-pycharm and released an alpha so it can be tested and improved upon. (Which is why this issue was created in the first place.)

For us the ship has sailed, with --msg-template we can't implement it in a backwards compatible manner, same here for the JSONReporter.

The old format for the json reporter can't be changed but the new one in 3.0 alpha (if we want to do a breaking change) are not set in stone.

IMO breaking changes should be limited to the absolute minimum required to stay sane. If there is a workable alternative, that should almost always be preferred.

Agreed.

On the other hand optparse is nightmarish, and not refactoring also has a cost (bugs, bugs not getting fixed, inconsistencies and oddities when using the CLI, plus of course lost of will to live among maintainers 😄).

The json change is a breaking change, because it's a bug that was here for so long that it became a feature see #3514.

For 3.0 we'll have to break some things, hopefully not too much and hopefully we'll provide a migration tool or at least documentation for the migration (especially regarding the evolution in pylintrc). There already is some changes we could automate (blacklist => disallow-list).

[cdce8p]

The old format for the json reporter can't be changed but the new one in 3.0 alpha (if we want to do a breaking change) are not set in stone.

In theory true, but what I'm advocating for is that there should be an existing alternative before we add a breaking change. Tools are not able / willing to check the version before using the output. My change to vscode-python was almost rejected because of it. Thus if we break the json output, tools will just stop working. That's not ideal for anyone. There needs to be a considered approach for it, preferably with a long deprecation period in between.

This isn't possible here. Which is why I'm advocating for a separate reporter option. The most that would possible IMO would be to require --score=n to get the old output in 3.0. That would at least be backwards compatible and tools can add it to their config options.

On the other hand optparse is nightmarish, and not refactoring also has a cost (bugs, bugs not getting fixed, inconsistencies and oddities when using the CLI, plus of course lost of will to live among maintainers 😄).

I agree, but that is at least somewhat different. It should only effect plugin developers which can more easily adapt. E.g. by specifying a supported pylint range in their requirements. A (short) deprecation period would be nice nevertheless.

For 3.0 we'll have to break some things, hopefully not too much and hopefully we'll provide a migration tool or at least documentation for the migration (especially regarding the evolution in pylintrc). There already is some changes we could automate (blacklist => disallow-list).

Yeah it's a breaking change, but not a big issue. As I explained earlier, the difference here is that there is an existing alternative. It won't be a hard break from one day to another with not migration path for tools that need to support multiple pylint versions.

[Pierre-Sassoulas]

there should be an existing alternative before we add a breaking change. Tools are not able / willing to check the version before using the output.

Makes sense. Let's start doing long deprecation period when required.