Car docs bot: diff on generated readme file

[marcellofuschi]

Closes #32690

• Doesn't use pickle, it just compares the markdown files (the one in master and the one in a PR).
• Only the car docs table is compared. The rest of the file is ignored.
• It relies on the (Make, Model) pair from the table to identify a row. If a column value was changed but the pair remained the same, that's shown as a column change in the PR comment (as opposed to an addition+removal).
• Adds the Detail sentence as an expandable button in the docs (like "Hardware Needed").

Here's an example result: marcellofuschi#2 (comment)
Detail sentence isn't visible in the example because that column is not in master yet.

[github-actions]

Thanks for contributing to openpilot! In order for us to review your PR as quickly as possible, check the following:

• Convert your PR to a draft unless it's ready to review
• Read the contributing docs
• Before marking as "ready for review", ensure:
  • the goal is clearly stated in the description
  • all the tests are passing
  • the change is something we merge
  • include a route or your device' dongle ID if relevant

[marcellofuschi]

Need to fix: the code for parsing the markdown table is too naive and doesn't handle potential escaped pipes. Will do it in a bit.

[sshane]

The sentence is not very easy to read, is there something we can do about that? Tooltip, comment? We don't currently show it, so it's not a requirement to show it now.

And does this satisfy this point in the issue?

Changes to column values should be picked up as a column change, not addition and removal. GitHub already does that for us!

[marcellofuschi]

The sentence is not very easy to read, is there something we can do about that?

Pushed a change that should improve that. I automatically add some   to the column value so that once rendered the table prefers overflowing with scroll (up to a certain point). Here is the result.

And does this satisfy this point in the issue?
"Changes to column values should be picked up as a column change, not addition and removal. [...]"

I think so, as long as the changes aren't made to the columns I use for identifying a row (i.e., Make and Model). See here for an example.

---

I also added a length limit to the generated comment, so that it won't show more than 10 rows per category, with an indication of the number of hidden rows (e.g., + 31 other rows). I believe the comment would get truncated anyway if too long so it's better to handle it more gracefully.

---

Need to fix: the code for parsing the markdown table is too naive and doesn't handle potential escaped pipes.

Still gotta fix this.

[sshane]

Getting this, why doesn't CI catch it?:

batman@workstation-shane:~/openpilot/selfdrive/debug$ python3 print_docs_diff.py 
  File "/home/batman/openpilot/selfdrive/debug/print_docs_diff.py", line 22
    results = {f'{r['Make']} {r['Model']}': r for r in results}
                     ^^^^
SyntaxError: f-string: unmatched '['

I would say it's okay to hide the sentence for now, it changes the style of the table too much and pushes the other columns near the end off the screen. Another user put it in the markdown file as comments, with some small special handling.

The current differ makes use of the platform name to detect model year/name changes, which while would be nice to keep, are a bit more difficult if we just have the column values, so I'm fine with losing that ability for now.

I see some re.subs, it's important to do the simplest thing first. I would use a str.replace if it's a drop-in replacement for example.

[sshane]

It makes more sense if new_docs_content is hardcoded using BASEDIR + CARS.md file locally, and base_docs_content should be passed in as a path, using the base commit on the PR (downloaded in the Action)

[marcellofuschi]

I made the change (ce8c77c) and tested it in my fork

[marcellofuschi]

I would say it's okay to hide the sentence for now, it changes the style of the table too much and pushes the other columns near the end off the screen. Another user put it in the markdown file as comments, with some small special handling.

This was partly because I had also enlarged the Supported Package column. I reverted it now, so the table should look a bit more familiar. Let me know if this could work.

Regarding re.subs, the first one would be the easiest to remove. However, I believe it's more robust to check for a word boundary before the appearance of assets/ when performing the replacement. Let me know if you agree; otherwise, I'll change it to .replace('assets/', 'https://media.githubusercontent.com/media/commaai/openpilot/master/docs/assets/')

I'll look into the other points you raised. Thanks for the feedback.

[marcellofuschi]

Getting this, why doesn't CI catch it?:

I reproduced the error, I think it has to do with wrong quotes. Demo example:

I've fixed it in the code, so it should be fine now. I'm not sure why CI didn't catch it.

[sshane]

We want to get rid of the two checkouts in this PR and just use curl/wget

[marcellofuschi]

Done (ddbe964)

[marcellofuschi]

Rebased after the recent changes, and tested again here

The Detail sentence is missing from the docs because docs_definitions.py was moved to opendbc, so the generated diff doesn't currently include Detail sentence.

I think it looked fine when we added it to the table as a new column (with formatting done through   usage). In case you agree, I opened a PR to opendbc to re-add it: commaai/opendbc#1182

This way, Detail will also be involved in the diff.

[github-actions]

This PR has had no activity for 9 days. It will be automatically closed in 2 days if there is no activity.

[github-actions]

This PR has been automatically closed due to inactivity. Feel free to re-open once activity resumes.