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 support to open the online message documentation as quick fix action #298

Merged
merged 3 commits into from
Mar 13, 2023

Conversation

DudeNr33
Copy link
Contributor

The Pylint community put in a lot of work to document the messages with good / bad code examples and optionally additional information (see pylint-dev/pylint#5953).

Now that the majority of messages are covered (and the uncovered ones have at least placeholders), I think it would be really nice if users of vscode-pylint could directly open up this documentation in VS Code.

This PR adds a new quick fix code action that is available for every message and allows to open up the corresponding link in the online documentation for the respective message in a SimpleBrowser view:

image

Let me know what you think!

@DudeNr33
Copy link
Contributor Author

@microsoft-github-policy-service agree

@DanielNoord
Copy link

Just voicing my gratitude for this idea. I never thought about this but I think it is a really nice benefit of all the work we put in and is also likely to generate more interest in making the examples even better/informative! Hope that this makes it into the plugin!

@karthiknadig karthiknadig self-assigned this Mar 13, 2023
@karthiknadig karthiknadig self-requested a review March 13, 2023 17:53
@karthiknadig karthiknadig added the feature-request Request for new features or functionality label Mar 13, 2023
@karthiknadig
Copy link
Member

@DanielNoord Do we need to be version specific when opening the link? we do capture version information on LS load, so we should be able to re-use that information.

@karthiknadig
Copy link
Member

@DudeNr33 What do you think about using the code_description field on the diagnostic itself rather than a code action:

image

image

@karthiknadig
Copy link
Member

Here is an example from rust:
image

This adds a link to the error code itself:
image

@DudeNr33
Copy link
Contributor Author

@DanielNoord Do we need to be version specific when opening the link? we do capture version information on LS load, so we should be able to re-use that information.

Not every version is available online, so that will be problematic:
image

Furthermore, older versions of the documentation don't have the message documentation in the required format. It is available only from v2.14.5 onwards. Plus, it has been an ongoing effort, so the older versions are not as complete as the latest version.
Messages are also not that "volatile", i.e. the documentation for message X on the latest branch will also apply to older versions as well.

The only case where the version information could be useful is in case some message was removed entirely. However in the proper way to handle this on Pylint side would be to keep the documentation link and include a "Removed in version x.y".

But I am also interested in Daniel's or @Pierre-Sassoulas opinion on this.
Maybe it would at least be a good idea to link against the stable version instead of latest.

Thank you for the suggestion with code_description, I will give that a try!

@Pierre-Sassoulas
Copy link

We should show the latest version all the time and we have an ongoing effort to keep the documentation for deleted messages in pylint. Not showing anything if the page does not exist seems safer (almost everything is documented but not everything yet).

@DanielNoord
Copy link

DanielNoord commented Mar 13, 2023

@DanielNoord Do we need to be version specific when opening the link? we do capture version information on LS load, so we should be able to re-use that information.

You shouldn't... We have set it up so that deprecated message names will have a short overview of where to go to for the new message, see:
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/len-as-condition.html
https://pylint.readthedocs.io/en/latest/user_guide/messages/convention/missing-docstring.html

So, as long as we keep reminding ourselves that we should provide redirects whenever we change the documentation folder layout you should be good with latest. Obviously you could use a tag, like https://pylint.readthedocs.io/en/v2.17.0/user_guide/messages/convention/missing-docstring.html, but we are committed to keeping redirects in our docs. If anything ever breaks that is then on us and likely something we can easily fix.

Edit: Oh wow, it's pylint maintainer rush hour I guess 😄 I think we all share the same sentiment: latest should be fine as we are committed to keeping old links at least "useful" and therefore they should always link to something that helps the user along.

@DudeNr33
Copy link
Contributor Author

@DudeNr33 What do you think about using the code_description field on the diagnostic itself rather than a code action:

image

image

I tried it and it would be equally easy to implement.
The main difference is that CodeDescription opens the link to the documentation in the default browser outside of VS Code.

I'm fine either way. The CodeAction way is nicer integrated into VS Code, but the CodeDescription seems to be the cleaner approach / the more intuitive place for the documentation link to live.

@karthiknadig
Copy link
Member

I recommend the codeDescription approach, as that is the intended purpose of the field. VS Code recommendation for code actions is that it should result in source code changes either via edits or commands.

@DudeNr33
Copy link
Contributor Author

Updated to use codeDescription instead!

@karrtikr karrtikr merged commit 547ee7e into microsoft:main Mar 13, 2023
@DudeNr33 DudeNr33 deleted the open-docs-code-action branch March 14, 2023 16:48
@karthiknadig
Copy link
Member

@DudeNr33 This is now available in pre-release version 2023.5.10731011.

@DudeNr33
Copy link
Contributor Author

That was fast, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants