Create separate documentation page for each message

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	📜 Docs

Description

Closes #1056, Closes #2235

Little thing I have worked on. Pages can definitely be improved but I think this is a good start and will help direct users to our documentation via google etc.

[coveralls]

Pull Request Test Coverage Report for Build 1758434731

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.01%) to 93.865%

---

Totals
Change from base Build 1758302345:	0.01%
Covered Lines:	14765
Relevant Lines:	15730

---

💛 - Coveralls

[Pierre-Sassoulas]

Did you know about https://github.com/vald-phoenix/pylint-errors ? Pretty cool project even if incomplete. Result : https://vald-phoenix.github.io/pylint-errors/plerr/errors/basic/C0102. Often it's the first result when searching for a particular message.

@DanielNoord What would you think about integrating this seriously in this MR ? @vald-phoenix what is your opinion about being integrated in pylint directly ?

[DanielNoord]

I think there are certainly benefits to be achieved when we can combine the two!

[cdce8p]

Some notes, although I didn't review the PR.

• I'm not sure that it makes sense to create separate pages for the different message categories. As a user I would much rather prefer one long list (maybe with sections) that contains all. Then use search to find the one I'm looking for.
• This PR does not change it so it can be ignored. I just wanted to mentioned that it probably makes sense to keep the old pages around. That way we don't invalidate existing links.
• I would also recommend some more discussion about the link template for individual messages. It would be good to get them right the first time as they will be difficult to change later. Some things to consider
  • What if we rename the message? It should probably be accessible via the old and new one.
  • If the id is used, what if that changes?
  • Should the checker name be part of the url template?
  • Does it make sense for the severity to be part of it?
• I like the use of a custom sphinx extension. I believe I've previously seen code related to how settings and error message are displayed in the docs directly in pylint. That might be something to look into when it comes time to refactor those areas. Moving them to an extension as well could be beneficial.

[DanielNoord]

• I'm not sure that it makes sense to create separate pages for the different message categories. As a user I would much rather prefer one long list (maybe with sections) that contains all. Then use search to find the one I'm looking for.

I will change this.

• This PR does not change it so it can be ignored. I just wanted to mentioned that it probably makes sense to keep the old pages around. That way we don't invalidate existing links.

👍

• I would also recommend some more discussion about the link template for individual messages. It would be good to get them right the first time as they will be difficult to change later. Some things to consider

  • What if we rename the message? It should probably be accessible via the old and new one.

Can we make redirects in read the docs? We could add redirects for old names. Although for missing-docstring this might be troublesome as we would need to redirect to 4+ pages. I'll have a look at the possibilities there.

• If the id is used, what if that changes?

This would require us to mark id's as being used even if they have changed. I don't think we currently store that information, we only store the name for old-names. Do we want to start doing that from now on?

• Should the checker name be part of the url template?

I don't think so? Even more things to keep track of when deprecating or changing stuff. Normally as user doesn't know which checker class emitted a warning anyway. I included it mostly for development purposes.

• Does it make sense for the severity to be part of it?

I'm not sure. Would it give users much more information besides what they currently have? I don't think it will help them with finding the page quicker than they can now, but it won't allow us to easily change the severity of messages. Including it on the page itself, rather than only as part of the id is something we could do though.

• I like the use of a custom sphinx extension. I believe I've previously seen code related to how settings and error message are displayed in the docs directly in pylint. That might be something to look into when it comes time to refactor those areas. Moving them to an extension as well could be beneficial.

There are two other extensions, although they are not as large as this one. I hope with the move to argparse (or another library) we can also easily generate the documentation for the settings as well 😄

[Pierre-Sassoulas]

Can we make redirects in read the docs?

We can but it's manual and I'd rather not have to do manual configuration for each name change.

If the id is used, what if that changes?

I don't understand the question, if an id existed at any point, it's a current ID or an old names, so 'id' are always "used" once they are added to pylint.

Should the checker name be part of the url template?

Let's keep this simple for us so we can move messages around internally if we need to for perfirmance as well as cleanliness. Some issues are super hard to fix because of messages being mixed together, we need to be able to burst the checkers (#4574)

I hope with the move to argparse (or another library) we can also easily generate the documentation for the settings as well

🙏

Regarding the template I like what @pheanex did here : https://pheanex.github.io/pylint/#R1705 I think it's the basis for https://github.com/vald-phoenix/pylint-errors too. I think we can add old_names handling to it and the work intensive part will be a proper description for all message. @vald-phoenix how would you feel if we take the current content of your repository for a start ?

[DanielNoord]

Can we make redirects in read the docs?

We can but it's manual and I'd rather not have to do manual configuration for each name change.

Can't we pass it to Sphinx or read the docs during the creating process? That would allow doing to from old-names.

I don't understand the question, if an id existed at any point, it's a current ID or an old names, so 'id' are always "used" once they are added to pylint.

Ah, I forgot id's are also stored in old names. That would solve this problem as we can just make it part of the redirect (if we can make it work).

Regarding the template I like what @pheanex did here : https://pheanex.github.io/pylint/#R1705 I think it's the basis for https://github.com/vald-phoenix/pylint-errors too. I think we can add old_names handling to it and the work intensive part will be a proper description for all message. @vald-phoenix how would you feel if we take the current content of your repository for a start ?

I'm not sure how easy it would be to transform those .md file to .rst. I have been thinking about extending this to allow storing information about messages in a separate directory that this extension would read from. For storing longer descriptions or explanations json files would work well and can be easily loaded. For code blocks json won't work. We could do something with a directory for each message which includes extra-information.json, good-code.rst, bad-code.rst`. Those good and bad code files can be parsed programatically from @valid-phoenix's project if they agree on merging his project.

[Pierre-Sassoulas]

We could do something with a directory for each message which includes extra-information.json, good-code.rst, bad-code.rst`.

Wild idea, but maybe we could take the good example / bad example from functional tests and convert it to .rst. It would force (some) standardization for functional test too. Putting the whole functional test content is not desirable but requiring the basic example with good code bad code would nudge us to have some structure:

Ie: having a nan-comparison message requires a tests/functional/n/nan_comparison/ directory containing:

tests/functional/n/nan_comparison/good.py
tests/functional/n/nan_comparison/bad.py
tests/functional/n/nan_comparison/bad.txt
tests/functional/n/nan_comparison/full.py
tests/functional/n/nan_comparison/full.rc
tests/functional/n/nan_comparison/full.txt
tests/functional/n/nan_comparison/issue_49569.py
tests/functional/n/nan_comparison/issue_49569.txt

The full.py or issue_49569 would contains the current functional tests for nan-comparison but would not be required or used for the documentation, only good and bad would.

The tests/functional/ext directory is close to that following #5374 (there is a directory with the extension name like tests/functional/ext/bad_builtin/)

[DanielNoord]

I like the idea but think it would be better to do another PR for it. Let's focus this PR on:

• Make one page with links to all messages
• Approve initial page template with minimal required information
• Approve links to individual pages
• Set up system for redirection for old names

[cdce8p]

We could do something with a directory for each message which includes extra-information.json, good-code.rst, bad-code.rst`.

Wild idea, but maybe we could take the good example / bad example from functional tests and convert it to .rst. It would force (some) standardization for functional test too. Putting the whole functional test content is not desirable but requiring the basic example with good code bad code would nudge us to have some structure:

Ie: having a nan-comparison message requires a tests/functional/n/nan_comparison/ directory containing:

tests/functional/n/nan_comparison/good.py
tests/functional/n/nan_comparison/bad.py
tests/functional/n/nan_comparison/bad.txt
tests/functional/n/nan_comparison/full.py
tests/functional/n/nan_comparison/full.rc
tests/functional/n/nan_comparison/full.txt
tests/functional/n/nan_comparison/issue_49569.py
tests/functional/n/nan_comparison/issue_49569.txt

@Pierre-Sassoulas I would not recommend this. It just adds too much additional complexity to the tests.

Different idea. In essence, this PR will already create separate page for each message. We could add those pages to git and manually add good examples were needed. The sphinx extension would only need to be able to handle custom sections in existing message rst files and don't delete them when updating.

That would also allow us to add additional explainations if necessary beyond those in the message dict.

[DanielNoord]

Different idea. In essence, this PR will already create separate page for each message. We could add those pages to git and manually add good examples were needed. The sphinx extension would only need to be able to handle custom sections in existing message rst files and don't delete them when updating.

That would also allow us to add additional explainations if necessary beyond those in the message dict.

I don't think this is a good idea. The other extension does this as well (ie, a base file that's overwritten by the extension) and this is really annoying to work with when improving or working on it. The extensions can be run as a script to see if the .rst files they create are correct and this would mean you would need to revert your changes every time you do a test run.
That doesn't sound too time consuming, but while working on this it became annoying pretty fast to have to constantly revert changes to some files while keeping the changes to the extensions files. It's much easier if we can just ignore all files in the doc/messages/ directory.

If we don't want to add these files to the tests directory I would propose to create a new message_pages_data directory somewhere in docs where we store some type of data to be used by the extension to create the .rst files.

[Pierre-Sassoulas]

It's much easier if we can just ignore all files in the doc/messages/ directory.

I agree with @DanielNoord here, I would favor a fully generated documentation based on files that we need to define now, preferably with no duplication. I don't think it's "additional complexity", it's "stricter structure" and "more automation". Forcing to have a functional test with a good/bad example in a per-determined directory enforced by tests will make some work at first (Some messages do not have functional tests.) then will automate most of the review and guarantee that we'll have an understandable example and a proper documentation. Re-using the functional test will make sure that the example is working and that there is no duplication between doc/tests. Also, I feel like I've spent hours of my life asking for functional tests in reviews 😄

Saw we want to document bad-option-value from Pylinter, we'd have.

• tests/functional/b/bad-option-value/ or tests/functional/pylinter/bad-option-value/ and inside:
  • good.py exists.
  • good.txt does not exists.
  • bad.py exists.
  • bad.txt exists.
  • More complete functional tests (but it's not mandatory)
• doc/user_guide/messages/bad-option-value.json or doc/user_guide/messages/pylinter/bad-option-value.json exists and contain information about the message stack-overflow question explaining it, merge request that introduced it, merge request that modified it, others (?)

Then we create a script / sphinx extension to generate the final .rst using the message description from the message store, the functional test and the doc additional information.

In the long run, the only things that would be more complicated than right now is that you have to create the json and make sure that you have two files in the functional tests containing a small understandable example.

[sprytnyk]

@Pierre-Sassoulas if pylint-errors lib will help pylint then you can do whatever you want with it. I don't mind merging :)
This is a dirty script I used to extract messages.
https://gist.github.com/vald-phoenix/8b05f5ffbf27225c36274d78b9cb930d
I used pandoc to convert rst files into markdown. I think you can do the reverse process as well.

[DanielNoord]

Sorry for the commit spam. My local environment doesn't really like running sphinx for some reason, so I need the read-the-docs action to check changes.

I have added a system to redirect for old names. It can be seen in action for missing-docstring here:
https://pylint--5396.org.readthedocs.build/en/5396/messages/convention/missing-docstring.html

Since all pages need to be referenced in an index, I have also added the renamed messages to the general messages list.
I wonder if we need the introduction pages anymore. I don't know what else to put on it and right now it is a bit empty. Anyone got any good ideas about what to add to the "messages introduction" page?

As I said, I think this PR should focus on:

• Make one page with links to all messages
• Approve initial page template with minimal required information
• Approve links to individual pages
• Set up system for redirection for old names

These have now all been added so this is ready for review. A follow up issue/PR should then deal with code examples and how we are going to save those.

[Pierre-Sassoulas]

My local environment doesn't really like running sphinx for some reason

Did you try tox -e docs ? I'm going to review once pylint 2.12.2 is released.

[cdce8p]

Didn't get to it earlier. I wanted to add one more comment about the separate test files.

I've stated to understand your intention for it. Although I would suggest to limit the scale. The existing functional tests have a logic on their own which I would prefer not to change. Furthermore, some files check multiple different, but related messages. Breaking that up would just complicate things.

Instead, what about create a separate test directory for doc examples? That way we could still validate them while keeping the existing once. Another benefit, the doc examples could be more precise and to the point. Eg. there isn't really a need to have regression tests in the there. At first glance this might sound like duplication, but I'm not sure we even need examples for every message. Most messages are pretty self explanatory.

[Pierre-Sassoulas]

what about create a separate test directory for doc examples?

This is a lot clearer than what I had in mind. Let's do this. Nice thing about it is that we'll have json and example in the same directory which will help comprehension a lot. I started to enforce one message / one functional test directory and there are a lot of exception (pylint / astroid crash type error for example, we hope there are none), and some messages do not have any tests. Which is a problem that we probably want to fix but it's also time consuming. And as you said some functional tests are multiple message mixed together which is not worth fixing.

[Pierre-Sassoulas]

We can make pylint.message classes better if you need helpers that do not exists yet. It should contains all the information regarding the message we just need to add new API to get it :)

[DanielNoord]

Did a force push to make sure the demo documentation generated by this PR is up to date with main and doesn't run into any issues.

[Pierre-Sassoulas]

Does this still need review ? I think this is a high priority MR because it does not need to wait for a release to be useful, we could be setting up GitHub pages right now.

[DanielNoord]

I think @cdce8p wanted to a final check to see if he agreed on the proposed links and how we link to renamed messages. That's why I added the Needs review label. Other than that I think this is indeed good to go.

@Pierre-Sassoulas Perhaps this is something to name in the summary you always put at the top of a new release. Would be helpful if we got some feedback from people of what they are missing/looking for on these pages.

[Pierre-Sassoulas]

Perhaps this is something to name in the summary you always put at the top of a new release.

Sure we're going to advertise it. Especially if we populated it with pylint-errors content it's definitely going to be a highlight ! But once it's the first result when searching for a message it's going to bring contributions too and the sooner we start "SEO" the better imo.

[DanielNoord]

@cdce8p Pinging you as I think @Pierre-Sassoulas makes a fair point about SEO and we're nearing the release of 2.13. Do you know if you'll have some time to review this in the coming days?

[DanielNoord]

Blocked by #5730. Afterwards I need to remove the reference to check_docs in this PR.

[Pierre-Sassoulas]

We merged #5730, removing the blocked label.

[DanielNoord]

Code updated!

[Pierre-Sassoulas]

The doc job fail becaus eof this warning: non-ascii-file-name.rst:12: WARNING: Inline emphasis start-string without end-string.

[DanielNoord]

Fixed! We can't allow \n at the end of a message description. The .rst becomes invalid. We could separate that into a different PR but I think it is minor enough to stay here.

[Pierre-Sassoulas]

I'm going to merge, if something need to change we can deal with it in a follow-up. This is nice enough that I'm pretty sure it's useful enough and to not delay too much. I'm quite excited to have an official documentation for each message. Amazing work Daniel !

It's lost in the review comment but the follow-up is #5527.