Improve JSON output format with score #4741
Comments
Pierre-Sassoulas
added
Breaking changes for 3.0 🦤
Enhancement ✨
|
@DanielNoord Mentioned this issue to me here. As already noted, it's not really possible to add the score to the existing This might not be as clean but it wouldn't break existing tools. What if we add a second Although an inconsistency, I would recommend not to change the default for For the output I would suggest a slight modification. Having {
"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"
}
]
} |
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 To me it makes sense to do a breaking |
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 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.
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.
The default output should not change otherwise we again start to break stuff unnecessarily.
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. |
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.)
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.
Agreed. On the other hand 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). |
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
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.
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. |
Makes sense. Let's start doing long deprecation period when required. |
|
@Pierre-Sassoulas I have a branch for this which works. Just one question about the transition. What plan do we want:
My preference would be 2 as it is the easiest to deal with in 5 years time but that would require some user interaction when upgrading (if they want to keep using the old format). |
|
What about creating a new json-future or new-json reporter and keeping the old one indefinitely ? (This was the plan to release in 2.x) |
I don't really like "future" or "new" as you could then get The new format has everything the old format had, just under a new name |
|
Users don't like to have to upgrade their json parsers though 😄 what about just increasing the value 'json2' later 'json3' if required ? |
|
Yeah I know, that's why I was hesitant to force them to migrate. I'm fine with Do you want to raise a |
|
I don't know if we even need to do that. Maybe a warning if the score option is yes and the parser is json ? " 'score' is not avaialble in this reporter use json2 insteador set score=no to remove this message." |
|
It's just that I don't really want to maintain the old reporter after we have the new one. There is no downside to using the new one and there is very little in their interface that they can share. |
|
Yeah I agree, no maintenance on the json reporter. This is a soft deprecation though we're not going to remove it either. Not sure how to convey that nuance (as evidenced by the cpython discussion on deprecation this is not yet a 'one true way to do it' topic) |
I was actually thinking of removing it 😅 Since there is a clear migration path and no loss of functionality I don't think it is too bad to actually remove it. What do you think? |
|
It forces a user actions on something that worked for them if they did not need the score. We're going to need to answer question about it on stackoverflow and here: even for us it's going to be more work than simply letting it be (especially if we don't maintain it). |
|
Hmm, yeah on the other hand people might be asking questions about the old format and we need to direct them to the new one. Let's raise a |
|
Fair enough. Isn't it pendingDeprecationWarning for soft deprecation though ? |
|
Agree we should use PendingDeprecationWarning. |
To me this definition doesn't really fit what we (or I) are trying to convey. As I said in the PR I think we should have one way to do things (Python Zen etc.. haha). The I'm fine with postponing or not even removing the old reporter as long as there are no issues, but I do think that we should communicate that it is deprecated and that there is another and preferred method to achieve the same thing. |
My understanding (and Pierre's) is that deprecating something because it's not the way but indicating that there's a possibility it may be kept forever even though not maintained (like optparse) is |
In that link there is a direct quote:
So, I guess we shouldn't use it? 😄 |
|
Well, but if you adapt that mindset I think the conclusion then is don't warn/deprecate at all until there's a consensus to remove it. In other words, in the absence of consensus to remove, your options are PendingDeprecationWarning, which is possibly confusing, or not warning at all. Unfortunately for a muddled situation like this, that's as clear as we can get. |
Yeah, that's right. But, isn't there a consensus that |
|
My understanding of the current situation is that issuing |
Would this be better then? I'd just like to find a way to communicate 1) to users that aren't reading the changelog that there is now a better way to do stuff and 2) to tell new users that stumble upon the |
|
Probably UserWarning in that case. |
|
@Pierre-Sassoulas Would you be okay with raising |
|
Sure, I don't think the cpython discussion came to a consensus so we don't know what the consensus will be but we still need to act. |
|
For what it's worth, I would recommend to leave the old Consider that there are tools out there which aim to provide compatibility with a wide range of pylint versions. Not to mention that it will take time until the new one is even supported. The best example that comes to mind is vscode-pylint. |
That's fair. I guess there isn't really a strong consensus for a warning so I'll think of a way to document the superiority of |
|
For everyone (like me) ending up here in search for a one line solution to use in CI/CD: Shell: pylint -f json2 yourSickProgram.py | python3 -c "import sys, json; print(json.load(sys.stdin)['statistics']['score'])"For everyone looking to only get the score programmatically without all the messages: Python: """
Pipe standard output stdout to /dev/null temporarily
to avoid excessive output
"""
import os
import sys
import pylint.lint
# Save original pipeline
ogPipe = sys.stdout
# Create temporary pipeline to /dev/null
tmpPipe = open(os.devnull, 'w')
# Pipe stdout to temporary pipeline
sys.stdout = tmpPipe
# Run pylint on your file(s) and save score in variable
result = round(pylint.lint.Run(
['yourSickProgram.py'], exit=False).linter.stats.global_note, 2)
# Set stdout pipe back to original
sys.stdout = ogPipe
# Close temporary pipeline
tmpPipe.close()
# Print result
print(result) |
thanks for the code |
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.The text was updated successfully, but these errors were encountered: