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

simplified diff for PRs #33205

Closed
wants to merge 40 commits into from
Closed

Conversation

agniv-the-marker
Copy link

Description

Should resolve #32690. Uses the ideas from #32720.

The workflow now pulls the main CARS.md file with wget and passes it to the doc printer. The doc printer now uses diff which simplifies most columns. For detail sentence handling, updated docs_defns so that detail sentence was writable from the data in the row. Also simplified a function in docs.py (thought I could use it to generate the detail sentence) and updated the testing within dump car docs to get the main CARS.md as well.

Verification

Manually edited CARS.md on my machine, got expected results.

Copy link
Contributor

github-actions bot commented Aug 6, 2024

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

@agniv-the-marker
Copy link
Author

agniv-the-marker commented Aug 6, 2024

Hopefully the workflow issues go away. That's embarrassing.

Anyway, @sshane mentioned hoverable tooltips, which looks like its possible. It would require changing it to format the template into something like [make](## "detail_sentence"). Like this

@agniv-the-marker
Copy link
Author

Ok, so the PR workflow should be working now. I don't really understand what exactly is going on and why you can't just do something like:

- name: Get info
  run: | 
    mkdir -p /path/
    wget -O /path/CARS.md link
- name: Save diff
  id: save_diff
  run: |
    output=$(${{ env.RUN }} "python selfdrive/debug/print_docs_diff.py --path /tmp/openpilot_cache/base_car_docs")
    output="${output//$'\n'/'%0A'}"
    echo "::set-output name=diff::$output"

Otherwise it takes a bit of time on the - uses: ./.github/workflows/setup-with-retry step. Any advice greatly appriciated.

@agniv-the-marker
Copy link
Author

All right. @sshane please let me know how this looks. Sorry it took so long (and so many checks!).

@sshane
Copy link
Contributor

sshane commented Aug 6, 2024

Can you show a few example comments it might leave, testing a few diffs (column value change, adding/removing row, and detail sentence)? You might need to open a PR to your own repo to allow the workflow to run and comment

@agniv-the-marker
Copy link
Author

agniv-the-marker commented Aug 7, 2024

@sshane , should be fully done now. Updated it so that commons wasn't imported. Also updated the handling of the file so that icons were appropriately shown. The issue with displaying an example is that they're often just too long lol. Here's a short one linked below.

agniv-the-marker#4 (comment)

from grabbing a log, this is what the detail sentences look like untrimmed: https://github.com/agniv-the-marker/openpilot/actions/runs/10283427115/job/28457236785?pr=4

📖 Detail Sentence Changes

  • Sentence for Hyundai Kona 2020 changed!
    - openpilot upgrades your <strong>Hyundai Kona 2020</strong> with automated lane centering and adaptive cruise control <strong>that automatically resumes from a stop</strong>. This car may not be able to take tight turns on its own.
    + openpilot upgrades your <strong>Hyundai Kona 2020</strong> with automated lane centering and adaptive cruise control <strong>while driving above 6 mph</strong>. This car may not be able to take tight turns on its own.
  • Sentence for Kia Forte 2019-21 changed!
    - openpilot upgrades your <strong>Kia Forte 2019-21</strong> with automated lane centering and adaptive cruise control <strong>that automatically resumes from a stop</strong>. This car may not be able to take tight turns on its own.
    + openpilot upgrades your <strong>Kia Forte 2019-21</strong> with automated lane centering and adaptive cruise control <strong>while driving above 6 mph</strong>. This car may not be able to take tight turns on its own.
  • Sentence for Toyota RAV4 Hybrid 2017-18 changed!
    - openpilot upgrades your <strong>Toyota RAV4 Hybrid 2017-18</strong> with automated lane centering and adaptive cruise control <strong>that automatically resumes from a stop</strong>. This car may not be able to take tight turns on its own.
    + openpilot upgrades your <strong>Toyota RAV4 Hybrid 2017-18</strong> with automated lane centering and adaptive cruise control <strong>while driving above 19 mph</strong>. This car may not be able to take tight turns on its own.
  • Sentence for Toyota RAV4 Hybrid 2016 changed!
    - openpilot upgrades your <strong>Toyota RAV4 Hybrid 2016</strong> with automated lane centering and adaptive cruise control <strong>that automatically resumes from a stop</strong>. This car may not be able to take tight turns on its own.
    + openpilot upgrades your <strong>Toyota RAV4 Hybrid 2016</strong> with automated lane centering and adaptive cruise control <strong>while driving above 19 mph</strong>. This car may not be able to take tight turns on its own.

@agniv-the-marker
Copy link
Author

agniv-the-marker commented Aug 7, 2024

dislike how this is a little longer and could definitely be faster. would love advice on how to do that, though now with the new import restriction i think you have to build openpilot to get access to the detail sentence builder/relevant Enums, so I think this is ok.

in terms of shorter code, there are probably nicer more pythonic ways of dealing with the stars/videos than how i did it. definetly gets the job done (and i like using the same function twice in a cute way).

also i think parts deserves a bit more oopmh to it. like, iterating through them and seeing which ones got removed, added, and formatting it like

  • removed
  • kept, unchanged
  • added, new thing
  • new thing but we have more now +N
  • new thing is reduced -N

since there's also a buy tag it might be fun to check the delta of prices?

@adeebshihadeh
Copy link
Contributor

Closing, responded on Discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Car docs bot: diff on generated readme file
3 participants