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

HELP WANTED: Help copy content from pylint-errors over to our own documentation #5953

Closed
4 tasks done
DanielNoord opened this issue Mar 23, 2022 · 75 comments · Fixed by #9933 or #9936
Closed
4 tasks done

HELP WANTED: Help copy content from pylint-errors over to our own documentation #5953

DanielNoord opened this issue Mar 23, 2022 · 75 comments · Fixed by #9933 or #9936
Labels
Documentation 📗 Hacktoberfest Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Milestone

Comments

@DanielNoord
Copy link
Collaborator

DanielNoord commented Mar 23, 2022

A longstanding issue of pylint has been that there is little documentation about some of the messages and why they are emitted. For some messages the short message that is displayed is not enough to make explicit what needs to be changed or what is considered wrong.
With the closure of #5527 and the merge of #5934 we have now set up a system that allows us to do so!
We have also received the okay from @vald-phoenix to base this additional documentation on their wonderful pylint-errors project.

However, now we need some community help to write this documentation. If you'd like to help out and help future users of pylint understand what pylint is telling them, please continue reading.

Two examples of how the new documentation is supposed to look like can be seen [here](https://pylint.pycqa.org/en/latest/messages/error/yield-inside-async-function.html) and [here](https://pylint.pycqa.org/en/latest/messages/convention/empty-docstring.html).

How to create such pages? The process is quite straightforward.
Within the doc/data/messages directory you can find various subdirectories. These are the letters of the alphabet. Within these directories you will find directories for each message that pylint can emit. These directories contain the data from which these pages are build.

To help build a new page:

  1. Create a new directory for the message you are working on. For example doc/data/messages/a/abstract-class-instantiated.
  2. Look at the page for the message in the pylint-errors repo. For example here. If the message is not documented yet you will need to come up with examples yourself.
  3. Create a good.py document in which you include a short code example that shows of good code that won't trigger the message. You can base this on pylint-error.
  4. Create a bad.py document in which you include a short bad code example. On the line that is supposed to emit the message please add a # [abstract-class-instantiated] comment. See these examples.
  5. If necessary create a pylintrc file. Here you can include configuration options for pylint like loading of optional checkers. See doc/data/messages/c/confusing-consecutive-elif/pylintrc for example.
  6. If necessary create a details.rst file. Here you can place additional details in .rst format that will be displayed on the page. Note that some of the details included on the pylint-error pages are already included in our Message or Description sections.
  7. If necessary create a related.rst file. Here you can place .rst style links to external sources that are relevant for the message. Generally we do not include the links to Test Cases and the Issue Tracker that pylint-error provides as they can break easily.
  8. If you're basing yourself on the work in the pylint-errors repo it is appreciated if you add the following line to your commits:
    Co-authored-by: Vladyslav Krylasov <vladyslav.krylasov@gmail.com> (or any of the other contributors to that project you're basing yourself on!
  9. To make sure the code examples are correct, you can run pytest doc/test_messages_documentation.py.
  10. Create a PR. A changelog entry is not necessary :)

Two examples of how to do this can be found in #5934.

To make this work somewhat manageable please do not add more than 5 messages in one PR, as otherwise it will be impossible to review the changes correctly.

The big list of messages without any documentation:
As of the 2023-09-09.

Related pull requests:
#5992, #5994, #5995, #6016, #5993, #5996, #6150, #5997, #6026, #6086, #6000, #6088, #6021, #6002, #6090, #6164, #6166, #6020, #6104, #6103, #6167, #6105, #6106, #6107, #6156, #6157, #6131, #6009, #6139, #6083, #6092, #6200, #6203, #6204, #6237, #6159, #6243, #6250, #6115, #6248, #6249, #6339, #6341, #6342, #6343, #6340, #6344, #6345, #6262, #6263, #6264, #6265, #6080, #6079, #6206, #6245, #6205, #6158, #6114, #6114, #6201, #6160, #6149, #6238, #6152, #6153, #6133, #6244, #6230, #6232, #6151, #6247, #6132, #6239, #6240, #6472, #6699, #6679, #6460, #6697, #6630, #6637, #6698, #6472, #6586, #6541, #6472, #6669, #6618, #6690, #6619, #6620, #6621, #6659, #6576, #6574, #6540, #6700, #6590, #6701, #6591, #6583, #6562, #6585 #9042

#7897

(List of related pull request is not exhaustive)

@DanielNoord DanielNoord added Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors Documentation 📗 labels Mar 23, 2022
@DanielNoord DanielNoord pinned this issue Mar 23, 2022
@Pierre-Sassoulas
Copy link
Member

I agree that having community support for this would be awesome, but I think we're not ready for that yet. In my opinion we should do a script to do the grunt work of migrating from pylint-error. Then proper reviews of the current examples when we integrate them and creation of missing examples would be super helpful.

Here's what I think we need to do following #5527, we really need community help only at a few steps:

  • Merging git history of pylnt-error (or simply copy pasting) and moving all the relevant content in doc/data/messages/original (maintainer job)
  • Create a script to migrate pylint-errors format to our structure (maintainer job)
  • Add and check messages by manageable batch until there is nothing left of the "original" pylint-errors files
  • Add functional tests for good.py/bad.py if they exists (maintainer job)
  • Add content when pylint-error did not have any

@DanielNoord
Copy link
Collaborator Author

DanielNoord commented Mar 23, 2022

Here's what I think we need to do following #5527, we really need community help only at a few steps:

  • Merging git history of pylnt-error (or simply copy pasting) and moving all the relevant content in doc/data/messages/original (maintainer job)
  • Create a script to migrate pylint-errors format to our structure (maintainer job)
  • Add and check messages by manageable batch until there is nothing left of the "original" pylint-errors files
  • Add functional tests for good.py/bad.py if they exists (maintainer job)
  • Add content when pylint-error did not have any

Sorry, perhaps I should have discussed with you prior.

However, I don't think this is feasible. During the transfer of the two test issues I already noticed a mistake in the examples for pylint-errors (a bad code example that was actually good code), so just migrating all data doesn't really work. Besides, the work necessary to migrate their data structure to ours isn't really worth the effort (imo). For example, the data that is in their Rationale sections is a combination of the message that gets emitted by pylint, the message description that is in the pylint code and some of their own comments. Often the data that originally came from pylint will be outdated so we also can't automate the separation of these types of data into details.rst, related.rst and not useful.

Basically, if we want to migrate the data we would need to put in so much effort that it is probably much better to do it by hand and just do one review for each message instead of doing multiple across many different steps.
Having no code example is better than copying incorrect code examples I think, so we would also need to check the data that gets migrated in any automated step...

That's why I added the line about adding co-authorship: I do want to credit them for any inspiration we use in our documentation.

@Pierre-Sassoulas
Copy link
Member

I already noticed a mistake in the examples for pylint-errors

Yes I noticed that, this is why I proposed to review chunk by chunk. If the migration script is complicated / not worth it, it make sense to do it manually. Did you check the work that need to be done to automate this, or is this a wet finger kind of thing ? I could try a proof of concept to see if it works otherwise (once 2.13 is released of course).

@DanielNoord
Copy link
Collaborator Author

I already noticed a mistake in the examples for pylint-errors

Yes I noticed that, this is why I proposed to review chunk by chunk. If the migration script is complicated / not worth it, it make sense to do it manually. Did you check the work that need to be done to automate this, or is this a wet finger kind of thing ? I could try a proof of concept to see if it works otherwise (once 2.13 is released of course).

No this is wet finger. But if we're going to review by chunks anyway, why then not do this manually? I mean, anybody who is looking to contribute is free to use a script to make their PR, but I think it makes more sense to do one review for each message for a "final" page than do two or three (initial migration, update of test case, add missing content).

@Pierre-Sassoulas
Copy link
Member

I think it makes more sense to do one review for each message for a "final" page than do two or three

Sure, multiple messages in the same PR would be messy, let's do "chunk of one" 😄

anybody who is looking to contribute is free to use a script to make their PR

Maybe it could even be our script and save them a lot of time. I'm going to create a proof of concept to migrate one message from pylint-error to our new doc system (so contributor just have to review/fix and create a PR) and see how it goes. You did the algorithm yourself in the description, I'm going to follow that. The "co-authored by" part in particular look like it would need to be automated to be accurate because it's not that easy to check the git history.

@DanielNoord
Copy link
Collaborator Author

Maybe it could even be our script and save them a lot of time. I'm going to create a proof of concept to migrate one message from pylint-error to our new doc system (so contributor just have to review/fix and create a PR) and see how it goes. You did the algorithm yourself in the description, I'm going to follow that. The "co-authored by" part in particular look like it would need to be automated to be accurate because it's not that easy to check the git history.

Co-authors show in the GitHub interface, luckily 😄 One thing we should check and which is more difficult is that the directories have the correct names, you easily miss that on Github.
Let me know if I can update the OP to add a script people can use.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Mar 27, 2022

Thing like astroid-error are internal and there's nothing to do for the user. Maybe we should keep a list of such messages and treat them differently ?

Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Sep 17, 2023
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Sep 17, 2023
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Sep 17, 2023
Pierre-Sassoulas added a commit to Pierre-Sassoulas/pylint that referenced this issue Sep 17, 2023
Refs pylint-dev#5953

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
Pierre-Sassoulas added a commit that referenced this issue Sep 17, 2023
Refs #5953

Co-authored-by: Jacob Walls <jacobtylerwalls@gmail.com>
@Pierre-Sassoulas Pierre-Sassoulas removed Help wanted 🙏 Outside help would be appreciated, good for new contributors Good first issue Friendly and approachable by new contributors labels Sep 17, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.1.0, 3.0.0 Sep 17, 2023
@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 3.0.0, 3.0.1, 3.1.0 Oct 2, 2023
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.1.0, 3.2.0 Feb 24, 2024
@jacobtylerwalls jacobtylerwalls modified the milestones: 3.2.0, 3.3.0 May 7, 2024
jacobtylerwalls added a commit that referenced this issue Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Hacktoberfest Needs PR This issue is accepted, sufficiently specified and now needs an implementation
Projects
None yet
10 participants