psyinfra / onyo

text-based inventory system on top of git
ISC License
3 stars 5 forks source link

DOC/RF: add docstrings to `utils.py` and constants #616

Closed aqw closed 6 months ago

aqw commented 6 months ago

This PR is again eclectic, but is finally beginning to turn the corner to focus more on docstrings.

The PR includes:

When documenting the constants, I intentionally did not wade into the issue of resolving the clear discrepancy of the names of PSEUDO_KEYS and RESERVED_KEYS vs how they are used. That is on my TODO list, but looks to be a complicated mess to unravel.

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 95.40230% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 96.56%. Comparing base (9a6d9af) to head (4713fc0). Report is 1 commits behind head on main.

Files Patch % Lines
onyo/lib/utils.py 89.47% 2 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #616 +/- ## ========================================== + Coverage 95.85% 96.56% +0.71% ========================================== Files 67 66 -1 Lines 5640 5560 -80 Branches 1103 1072 -31 ========================================== - Hits 5406 5369 -37 + Misses 132 109 -23 + Partials 102 82 -20 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

aqw commented 6 months ago

Thanks for the review!

I've addressed your comments. The only open one is about the voice. But I feel comfortable with the idea that only the short summary needs to be in the imperative.

TobiasKadelka commented 6 months ago

Thanks a lot :)

I am fine with it all, and regarding the voice: It works obviously either way, it was just hard not to notice while reading trough it all.

From my side this PR can be merged as soon as the tests ran successfully :)

aqw commented 6 months ago

Thanks!

Ideally, I will wait for Ben to review this. Specifically my changes to yaml_to_dict() and write_asset_file().

Buuut we'll see if that responsibility survives the weekend. ;-)

Thanks again for your review!

aqw commented 6 months ago

I talked with Ben yesterday and got an in-person +1 to merge. :-)