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

Document reasoning for error or warning messages #9245

Closed
eirnym opened this issue Nov 18, 2023 · 12 comments
Closed

Document reasoning for error or warning messages #9245

eirnym opened this issue Nov 18, 2023 · 12 comments
Labels
Documentation 📗 Help wanted 🙏 Outside help would be appreciated, good for new contributors

Comments

@eirnym
Copy link

eirnym commented Nov 18, 2023

Current problem

Linter error have few examples of "good" and "bad" behaviour, but in almost all cases there's no explanation what is the problem, what is the reasoning behind this check and what are possible outcomes of "bad" behaviour. I use specific reports as an example, but the explanation of a problem is common for almost all sections in pylint checks.

To show off the problem I'll question a few warnings below. With additional explanation they could be reasonable to follow, but without this explanation and wording used they and many others are more controversial or it looks like an order for a programmer what to do. When I read standard library, I see that my code that is full of warnings looks way cleaner than the standard library. :)

  • C0115: Missing class docstring (missing-class-docstring)
  • C0103: Type variable name "EnumerationType" doesn't conform to predefined naming style (invalid-name)
  • R0903: Too few public methods (0/2) (too-few-public-methods)
  • R0902: Too many instance attributes (14/7) (too-many-instance-attributes)

C0115:

Documentation says: "Used when a class has no docstring. Even an empty class must have a docstring.". But why classes must have a docstring?

I understand that developers of this checker wanted to increase readability of the code and this is a good point.


C0103:

Documentation dictates, that I need to use T over Type suffix for TypeVar

For me personally T suffix like A or LP prefixes in Windows Naming Convention. This is machine-readable, may be useful in some use cases, but not in python, where readability matters. I don't want to "unpack" this letters to make my code "clean".


R0902, R0913 and similar:

it's not always possible to put everything into subclasses or sub-datatypes even you want to. Examples in documentation show situations where arguments could be well-structured, but it's not always a case.


The same with R0903, it shows what is "good" and what is "bad" and gives no explanation why do I need to do that.

There's a lot of situations where this rule doesn't work as intended. Some of them:

  • an ORM Model with a single utility method
  • stateful object with a single method, which can't be __call__ method for clarity or interface reasons
  • Dataclass with a single public method

Use case examples of where this rule doesn't work as intended:

  • I have a class which defines only a single method to process a request and it's simple and easy and I don't need more
    • Why class? because I need to hold some state before actual method will be called or between calls
    • Why class? because I need to use this object for multiple different instances
  • I have a ORM model which defines a single utility method to work with the class. I don't need more
  • I have a registry with a single "public" method to register something and use __getitem__ to retrieve the tartget.
    • Why can't I use __setitem__? because I need more parameters which breaks __setitem__ and __getitem__ contract.

Desired solution

Additionally to the existing explanation what is "good" and what is "bad", I suggest to:

  • Write a human-friendly explanation why good examples are better than bad examples. Especially if this is about naming.
  • Explain why "bad" behaviour is worse than "good" in most cases. Possibly there would be many different situations to explain, but in most cases it is worth a while.
  • If there's too many cases to explain, maybe it's a situation where a specific check should be reviewed, split or better documented.
  • Explain where "bad" behaviour is actually fine, and how to avoid actually harmful situations.
  • Refer to Python Zen and other python PEP documents to encourage developers to actually follow these documents.
  • Avoid using strong wording such as "must" to avoid dictation over encouragement. Even PEP8 doesn't over-use this word: I found the word "must" only in a few places. Because of this wording I see many developers do the opposite in commercial code.

Additional context

No response

@eirnym eirnym added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Nov 18, 2023
@Pierre-Sassoulas Pierre-Sassoulas added Help wanted 🙏 Outside help would be appreciated, good for new contributors Documentation 📗 and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Nov 19, 2023
@Pierre-Sassoulas
Copy link
Member

Make sense since #5953 is almost over now.

@DanielNoord
Copy link
Collaborator

I don't really understand this issue.

I agree that some of the descriptions aren't as clear as they can be, but we have infrastructure in place to write those descriptions. We should just write them 😅
This feels like a bit like "pylint shouldn't have any bugs"/"the pylint documentation isn't perfect". It doesn't feel really actionable and any actually actionable suggestions should probably just be separate PRs?

@DanielNoord
Copy link
Collaborator

Seeing as that @Pierre-Sassoulas gave a 👍 I'm closing this. We can re-open if @eirnym sees a good reason to do so!

@eirnym
Copy link
Author

eirnym commented Nov 20, 2023

This is a general/epic issue to critically review every single message. Some of them have reasoning, some don't. I highlighted a few to show an example and provided questions what I'd like to see covered in each of them.

I could create the same issue for each article which is not readable, but in that case all of them would be closed "as spam".

I agree, that you expect PRs, but in most cases I can't imagine the reasoning behind each issue, why rule works in that and not the other way.

@Pierre-Sassoulas
Copy link
Member

I'm on mobile so I can't craft the comment I planned. I think it's a duplicate of an issue I created to add a rational to the doc template (might link later). Basically modifying the template is actionable but making a doc's message better should be a direct PR proposal for this particular doc's message.

@DanielNoord
Copy link
Collaborator

This is a general/epic issue to critically review every single message. Some of them have reasoning, some don't. I highlighted a few to show an example and provided questions what I'd like to see covered in each of them.

I think such an issue doesn't really work: documentation can always be made clearer and they will never reach a state where they are "done" and we can tick them off the list. We'll always want to keep improving them.

I could create the same issue for each article which is not readable, but in that case all of them would be closed "as spam".

Agreed. If there is a common problem, for example "links to this site are broken", we can open an issue about a group of pages but otherwise it would likely be considered spam.

I agree, that you expect PRs, but in most cases I can't imagine the reasoning behind each issue, why rule works in that and not the other way.

I think we also might not want to go into that much detail. Behaviour changes and it is hard to keep documentation up to date. I would gladly discuss individual improvements in PRs but that should probably be on a case by case basis.

Issues should lead to PRs, they should be actionable and solvable. I don't think keeping this issue open will lead to more PRs being opened or to a concentrated effort to improve our documentation.

@eirnym
Copy link
Author

eirnym commented Nov 20, 2023

@DanielNoord

this is a standard what I'd like to see: https://typescript-eslint.io/rules/no-explicit-any/

and I don't see that #5953 goes any near that point

@DanielNoord
Copy link
Collaborator

@DanielNoord

this is a standard what I'd like to see: https://typescript-eslint.io/rules/no-explicit-any/

and I don't see that #5953 goes any near that point

Like I said above, please provide constructive specific feedback to which we can respond.

We have "Examples", "Further reading" and "Resources". The other headers fall under our "Description".
The code being executable is nice, but something that would have to be implemented in Sphinx, not in pylint.

While I understand that the link you provide is much better than what most of our messages have I don't think our current system has anything that prevents you from fleshing out a specific page to be as detailed as those of eslint.
"Make your documentation more detailed" is not really an actionable issue, that's something we always strive for but are not necessarily tracking.

@eirnym
Copy link
Author

eirnym commented Nov 21, 2023

Let's be more specific comparing given eslint-no-explicit-any page with for example too-few-public-methods. @DanielNoord, please reply if I understood you right and be more specific if not.

Following comparison doesn't tell how to it in visually as it can be done in many ways depending on formatting language and/or tooling. I'll go through eslint page, comparing it to pylint's. Most of these points are commented in my original post in section Desired solution.

  1. Tool messages like on eslint page are good in general
  • How to enable/disable/configure add a link to at least generic documentation. Usually first-comer searches for an explanation, not how to edit pylintrc or which parameters to add. this is the second intention when a developer wants to enable/disable or configure it.
  • pylint currently doesn't provide any automatic fixes, so this block can be omitted. It could be done by providing API and/or extendable API for automatic fixes. Many issues may be fixed automatically.
  • pylint may provide specific suggestions and/or have an extendable API for suggestions. Some of these suggestions I see in non-generic message.
  1. How to apply rule and increase/decrease severity is an important information. As a random example: I as a developer want to too-few-public-methods be an error in some projects, and in other I prefer it to be a warning or even a notice. This may (or may not) have an impact on scoring later on.
  2. Correct and incorrect examples. eslint uses extended UI here, and ReStructuredText may not have such capabilities. But my point is not in such presentation, but multiple examples themselves. Please, look on my note on RST and Sphynx at the bottom. Currently, pylint pages usually provide only one example and a single solution, without considering some other possibilities.
  3. Then there's options with examples how to configure this check. Most of pylint checks might have options to configure.
  4. Then goes an important section "When Not To Use It", describing when enabling this option could be actually harmful for code. This section should provide information a developer to decide if the option should be enabled/disabled globally or locally.
  5. Related to section is useful as it shows that few different lint messages are similar to each other.
  6. Further reading is one more place to add to the documentation, where this check originates from. Some parts of PEP8 refer to other PEPs and this is a good place too add not only PEP8, but other references as well.
  7. Resources section increases the awareness of the check and how check is good. Code and tests may provide additional information to the documentation provided.

Note about Sphinx and ReStructuredText.

Sphinx is a tool to take markup text in one format and then generate other format. I believe, @DanielNoord probably refer to limitations of ReStructuredText. Few years ago I started to use and moved all my documentation to AsciiDoc format as it gives me possibilities I need. ReStructuredText is good enough, but it not so flexible and not so extensible as AsciiDoc. As far as I know ReadTheDocs supports the asciidoc and there's tools to convert RST to AsciiDoc.

@eirnym
Copy link
Author

eirnym commented Nov 21, 2023

Even pylint stay with ReStructuredText, most of my points make sense for me and it's possible to increase standard level of pylint documentation. The only two design elements on eslint pages can't be represented in ReStructuredText are blocks on the top and example array. Each example can be separated out in a separate sections of some sort and in some cases additional notes should be added.

@DanielNoord
Copy link
Collaborator

Thanks for the detailed post! I'll reply to your comments.

* How to enable/disable/configure add a link to at least generic documentation. Usually first-comer searches for an explanation, not how to edit `pylintrc` or which parameters to add. this is the second intention when a developer wants to enable/disable or configure it.

I think that might be a nice suggestion. However, note that for all messages that are not enabled by default we already provide such instructions like here.
We also provide a Configuration file section there.

For plugin checkers we also have this, see here.

I'm -0 on adding this section to all messages that are turned on by default: pylint should have sane defaults and you shouldn't want to turn on each messages individually. That's not to say our defaults are perfect but we should work towards this.

I can see how disabling the message might warrant a section, but I feel like that might also just clutter the page. Why not add a link to the configuration explanation at the bottom of all pages?

* pylint currently doesn't provide any automatic fixes, so this block can be omitted. It could be done by providing API and/or extendable API for automatic fixes. Many issues may be fixed automatically.

This is not related to the documentation but a feature request. We already track it here #7438

* pylint may provide specific suggestions and/or have an extendable API for suggestions. Some of these suggestions I see in non-generic message.

This is very similar to the issue described above.

2. How to apply rule and increase/decrease severity is an important information. As a random example: I as a developer want to `too-few-public-methods` be an error in some projects, and in other I prefer it to be a warning or even a notice. This may (or may not) have an impact on scoring later on.

Again, not related to documentation but a feature request that is being tracked here #2293

3. Correct and incorrect examples. eslint uses extended UI here, and ReStructuredText may not have such capabilities. But my point is not in such presentation, but multiple examples themselves. Please, look on my note on RST and Sphynx at the bottom. Currently, pylint pages usually provide only one example and a single solution, without considering some other possibilities.

We have infrastructure to have multi-directory examples. The issue linked above tracks adding at least one example for each message which is a nice and actionable issue.
If you can come up with more useful examples, please create PRs to add those. In my opinion it doens't make sense to have an issue for "Add more examples" as we would never be able to close it as there is no definition of done.

4. Then there's options with examples how to configure this check. Most of pylint checks might have options to configure.

Those can and are often documented. If not we have infrastructure to do so, see this page.

5. Then goes an important section "When Not To Use It", describing when enabling this option could be actually harmful for code. This section should provide information a developer to decide if the option should be enabled/disabled globally or locally.

Again, something that can already be written down. Apparently we don't document this but there is no reason to make an issue for this.

6. `Related to` section is useful as it shows that few different lint messages are similar to each other.

A nice improvement, but I don't think we need additional infrastructure compared to the related links section we can already make such as here.

7. Further reading is one more place to add to the documentation, where this check originates from. Some parts of PEP8 refer to other PEPs and this is a good place too add not only PEP8, but other references as well.

Same as above, I don't think we need a section for this currently as it fits in Related links.

8. Resources section increases the awareness of the check and how check is good. Code and tests may provide additional information to the documentation provided.

Same as above, I don't think we need a section for this currently as it fits in Related links.

Sphinx is a tool to take markup text in one format and then generate other format. I believe, @DanielNoord probably refer to limitations of ReStructuredText. Few years ago I started to use and moved all my documentation to AsciiDoc format as it gives me possibilities I need. ReStructuredText is good enough, but it not so flexible and not so extensible as AsciiDoc. As far as I know ReadTheDocs supports the asciidoc and there's tools to convert RST to AsciiDoc.

I was referring to Sphinx as we use it to render the code blocks. To make the code executeable Sphinx would need to make this a feature. There is https://pypi.org/project/sphinx-exec-directive/ but it serves a different purpose.

Even pylint stay with ReStructuredText, most of my points make sense for me and it's possible to increase standard level of pylint documentation. The only two design elements on eslint pages can't be represented in ReStructuredText are blocks on the top and example array. Each example can be separated out in a separate sections of some sort and in some cases additional notes should be added.

I think all of your suggestions "make sense", they just don't warrant this issue being kept open. Most of them can already be done in our infrastructure we just haven't found the volunteers to do so. The other stuff we already track in our issue tracker.

Hopefully I have answered all your points, let me know if I missed any 😄

@Pierre-Sassoulas
Copy link
Member

I did not find the issue I was talking about, it seems we have a pretty nice template atm : It's not 1:1 with eslint or ruff as Daniel said, but it's equivalent and relevant to what pylint features currently exists. If a new section should be added please open a new issue. The message description is currently often used to tell WHEN the message is raised instead of WHY, if you want to open an issue for that, listing what message's descriptions need to evolve would make the issue actionable (a kind of todo list).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Help wanted 🙏 Outside help would be appreciated, good for new contributors
Projects
None yet
Development

No branches or pull requests

3 participants