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

Integrate content from pylint-errors into the new pages for message #5527

Closed
2 of 6 tasks
Pierre-Sassoulas opened this issue Dec 14, 2021 · 10 comments · Fixed by #5934
Closed
2 of 6 tasks

Integrate content from pylint-errors into the new pages for message #5527

Pierre-Sassoulas opened this issue Dec 14, 2021 · 10 comments · Fixed by #5934
Labels
Documentation 📗 Enhancement ✨ Improvement to a component

Comments

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Dec 14, 2021

Current problem

We'll have a single documentation page for each message following #5396 but we need to add the content.

Desired solution

Start by taking what exists in pylint-errors, the owner is okay with it and the license is MIT

  • Proof of concept of a structure and linking for the individual pages (Create separate documentation page for each message #5396)
  • Fix small issue with two titles that are the same (Duplicated title in pylint's message documentation #5740)
  • Merging git history of pylnt-error (or simply copy pasting) and moving all the relevant content in doc/data/messages/original
  • Create a script to migrate pylint-errors format to our structure
  • 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

Additional context

#5396 (comment)

@DanielNoord
Copy link
Collaborator

#5396 is more about creating a structure and linking for the individual pages. I think this issue and associated PRs should cover how we are going to store the data for the messages pages. Later PRs can then cover writing the actual data.

Based on the work of @vald-phoenix for pylint-errors I think each page should (ideally) cover the following sections:

  1. Rationale
  2. Good code example
  3. Bad code example
  4. Development relevant information (checker name, file link, etc.)
  5. Related sources

The first message on pylint-errors already contains most of this data for example, see example. We should definitely base the content of our pages on the data already collected there.

However, before doing the "data collection" part we should consider how to store these sections. I thought some more about integrating the code examples with the functional tests, but I think that is likely going to become a big mess. Taking the message I linked as an example: the good and bad code takes up 7 lines, whereas in our test suite this is 17 lines. For other messages this will be much higher. The file for arguments-differ is 320+ lines at the moment. I don't see how we can dim this done effectively while also maintaining a good test suite without substantial time investment.
Thus, I don't think we should mix the tests with the documentation code.

What I would propose is to create a new folder doc/data/messages. Within these each message name (so yield-inside-async-function, not E1700) get its own directory. This directory can contain 4 files:
rationale.rst, good_code.py, bad_code.py and sources.rst. The fourth section would be generated automatically.
These can then be taken as input to the sphinx extension and put within a predefined template. We could handle missing files gracefully, but also add new section easily by adding new_section.rst.

We can add many things to this to improve it. For example, adding information about writing checker documentation to the PR template and creating a "suggest documentation yourself" button for pages for which no code examples have been written.

One major drawback to this approach is that it is difficult to see for a contributor how rationale.rst would eventually look like on the documentation page. Therefore, I think it is crucial to write good documentation about writing documentation and show that the script can be run locally to get a preview of the final .rst files.
One major positive about this approach is that by separating the files we can add new sections easily and could even (at a later point) write some sort of tests for our code examples. (ie., iterate over all good_code and only enable the message in the folder name and see if no messages are emitted).

Please let me know what you think of my proposed sections per message, my proposed folder structure/location and my proposed file separation. I'd like to work on the actual implementation of this myself, but I think we should reach consensus on these topics before going forwards.

@Pierre-Sassoulas
Copy link
Member Author

let me know what you think of my proposed sections per message,

I like that the rational is coming first. Development related information imho should be after related source like stack-overflow or blog post.

let me know what you think of my proposed folder structure/location:

Perfect.

let me know what you think of my proposed file separation

All good, I don't see the "development related" information file ? Maybe it's entirely generated ?

(ie., iterate over all good_code and only enable the message in the folder name and see if no messages are emitted).

Agree with that, it's not hard to launch the functional tests on good.py/bad.py to check that it does what it's supposed to do (and it check that our examples and pylint are consistent which is really valuable). Let's do that in the next step though.

On a more technical note I think we should merge git history with pylint-error and create a feature branch in pylint so we don't have an enormous merge request with thousands of changes while we work on this.

@Pierre-Sassoulas
Copy link
Member Author

I think this issue and associated PRs should cover how we are going to store the data for the messages pages. Later PRs can then cover writing the actual data.

Do you want to create a new issue for adding data or do you re-scope this one and create a new issue about adding data ?

@DanielNoord
Copy link
Collaborator

I like that the rational is coming first. Development related information imho should be after related source like stack-overflow or blog post.

Works for me.

All good, I don't see the "development related" information file ? Maybe it's entirely generated ?

Yes!

Let's do that in the next step though.

👍

On a more technical note I think we should merge git history with pylint-error and create a feature branch in pylint so we don't have an enormous merge request with thousands of changes while we work on this.

I was thinking of doing one PR to set up the directory and file system and then doing subsequent PR's for smaller batches of code examples. While I trust that the information in pylint-errors is good we can't simply copy it without looking at each individual message.

Do you want to create a new issue for adding data or do you re-scope this one and create a new issue about adding data ?

We can re-scope this one and close when the file management is in place.

Then do a new issue "Copy data from pylint-errors and track progress" and afterwards do a "Add missing data to messages documentation" issue.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Dec 18, 2021

While I trust that the information in pylint-errors is good we can't simply copy it without looking at each individual message.

Sure ! What do you think of this ?

  • Proof of concept of a structure and linking for the individual pages
  • Merging git history of pylnt-error (or simply copy pasting) and moving all the relevant content in doc/data/messages/original
  • Create a script to migrate pylint-errors format to our structure
  • 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

@DanielNoord
Copy link
Collaborator

I'm not sure about actually merging the git history, but I think in general such a workflow should work!

@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Jan 28, 2022
@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jan 28, 2022

Each individual message is now live following the merging of #5396 ! 🎉

There's a slight problem with presentation as two title are the same:

double_titlze

Next step are in the checklist of the issue's description.

@DanielNoord
Copy link
Collaborator

There's a slight problem with presentation as two title are the same:

double_titlze

Next step are in the checklist of the issue's description.

I'll fix this asap. Could you create an issue and assign me? I'm on mobile now, but I'll try and get to this asap as not to influence SEO too much.

@Pierre-Sassoulas
Copy link
Member Author

Reopening because some of the item are not handled yet.

@DanielNoord
Copy link
Collaborator

Going to create a follow up issue to help do this and guide the effort. I think the task in this issue (set up a system) has been done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Enhancement ✨ Improvement to a component
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants