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 end_line and end_column to the output of the JSONReporter #5380

Closed
DanielNoord opened this issue Nov 24, 2021 · 3 comments · Fixed by #5456
Closed

Add end_line and end_column to the output of the JSONReporter #5380

DanielNoord opened this issue Nov 24, 2021 · 3 comments · Fixed by #5456
Labels
Enhancement ✨ Improvement to a component High priority Issue with more than 10 reactions
Milestone

Comments

@DanielNoord
Copy link
Collaborator

Current problem

These attributes were added in astroid 2.9.0 to most nodes and should be accessible from the output of the JSONReporter.

Desired solution

Basically all changes in this reverted commit should be committed to 3.0:

6e91225

Additional context

No response

@cdce8p
Copy link
Member

cdce8p commented Nov 30, 2021

I'm wondering if we could do this change earlier.

The last few days I've been working on the VSCode python extension to enable error ranges for pylint. The issue is that the --msg-template argument isn't (or wasn't until 2.12.0) backwards compatible. I.e. --msg-template={end_line} will crash on pylint < 2.12. Thus we can't use it version independently to also support ranges.

As I see it, the JSONReporter would be our best bet to implement it in a backwards compatible manner. With the existing text reporters, it's reasonable to assume that if user parse the output, any changes to the format will break it. In contrast, for JSON I would expect that most already use a json parser. So as long as we don't add any invalid values, adding two additional fields won't break this.

As the VSCode error range update depends on this (or an alternative), I would like to add a solution in time for 2.12.2.

What do you think @Pierre-Sassoulas and @DanielNoord? I'm also open to other ideas how to resolve this.

@DanielNoord
Copy link
Collaborator Author

I think it makes sense. The only issue I foresee now is when people use **output.items() on the output of the JSONReporter, but that is such an anti-pattern that I don't think we should really worry about supporting it.

I do wonder however how this relates to the breaking changes introduced with #3514 and #4741. I think it might make sense to see if we can prepare support for these changes in vscode now, or comment on those issue based on the experience you have with vscode about how to do that in the most non-breaking way.

@cdce8p
Copy link
Member

cdce8p commented Dec 1, 2021

I do wonder however how this relates to the breaking changes introduced with #3514 and #4741. I think it might make sense to see if we can prepare support for these changes in vscode now, or comment on those issue based on the experience you have with vscode about how to do that in the most non-breaking way.

Left a comment there: #4741 (comment). Basically I believe that it could be implemented without breaking existing use cases.

@cdce8p cdce8p modified the milestones: 3.0, 2.12.2 Dec 2, 2021
@cdce8p cdce8p added Blocker 🙅 Blocks the next release High priority Issue with more than 10 reactions labels Dec 2, 2021
@cdce8p cdce8p removed the Blocker 🙅 Blocks the next release label Dec 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component High priority Issue with more than 10 reactions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants