-
Notifications
You must be signed in to change notification settings - Fork 30
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
Support PEP 727 Doc objects #279
Conversation
All tests passed on Python 3.10... I guess we're still supporting unsupported versions of Python (3.7 and 3.8). |
c51afed
to
9cd1c9c
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 99.82% 99.86% +0.04%
==========================================
Files 33 33
Lines 2320 2282 -38
==========================================
- Hits 2316 2279 -37
+ Misses 4 3 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@brentyi : The Also, Python 3.7 end-of-life was June 27th, 2023. My unsolicited recommendation is that Tyro drop support for Python 3.7. Would you be open to a PR to drop 3.7 (and maybe 3.8) from the test matrix and update the trove classifiers in |
Thanks @emcd!! I went ahead and did the 3.7 changes in #280, since it enables some cleanup that's been on my mind for a while. For the rest of the PR:
|
Excellent. Thank you! (Technically, Python 3.8 has been EOL since last October and should be considered for removal too. But 3.7 was the immediate obstacle that I faced; glad to have that out of the way.)
Grok 3 also suggested that site and I initially tried intercepting there. However, the test that I added to
I mostly followed your previous PR for optionally turning off the helptext comments for this part of the change. But, if we're not going to use a marker, then I can revert the changes to the struct specs. However, I would argue that passing the markers rather than a specialized feature bool is more future-proof. But, your call.
Sure. I had put a warning on the new marker, but if we're dropping the marker, then I can make a separate section and transfer the warning there. However, I would like to present a unified view of the precedence of helptext sources: (1) specific metadata ( Btw, even though Jelle was the sponsor of PEP 727 and had recommended that it be withdrawn last July, a significant amount feedback in support of the PEP did continue to trickle in after the fact and it is still in Draft status. He also said in the PEP discussion that |
Co-Authored-By: Anthropic claude-3-7-sonnet, xAI grok-3
Ah yes, I agree that a feature bool isn't ideal. What I meant to suggest is to remove the ability to configure it altogether. It seems OK to me to leave this feature always on.
This makes a lot of sense. As one possible structure, how about creating a section explicitly for precedence? For example:
This way we can make the precedence rules explicit, while still putting the "recommended" ways to incorporate helptext first.
This was a helpful read, thanks! I'll confess I find myself more on the anti-PEP 727 side 😛 (but still happy to support it) |
Done - the PEP 727 feature is now always on. I had to add some exception handling to mitigate pathological cases where type hint inspection would fail (some Pydantic objects with weak local references, some objects with incomplete type signatures on Python 3.8). Making this always-on allowed us to catch those cases before someone else caught them in the wild. I left the
Done! Thanks for the feedback.
Haha. Thank you for being so gracious, @brentyi . But, seriously, if you do not want this feature in your codebase, I won't press the issue. Honestly, I've been thinking about writing a "simple" library that would support generating both documentation and CLIs purely from Anyway, I think I'm ready for another review when you have a chance. (Note that we have a sticky Python 3.7 test because I opened this PR before it was removing from the matrix (and presumably the project's required tests list).) (Also note that the Codecov API rate limits very heavily and that is where the Codecov failures have been coming from.) |
Thanks for the quick update @emcd!
Will certainly let you know if there's anything I don't want to support! (#254 is a contender 😆 but I haven't found time to properly think about it) I just did some refactor to reuse tyro's existing metadata inspection logic. With this implementation I think the maintenance burden should be extremely low => I'm happy to support If it looks okay to you I'll do a pass over the docs sometime this week and then merge 🙂 |
Not as quick as your refactor, but I guess you know the code better. ;)
😆
Looks good, though I swear adding logic to the generic stuct Thanks for the revisions. |
My guess is your original code would have worked if you swapped the order of the |
Co-Authored-By: Anthropic claude-3-7-sonnet, xAI grok-3
Closes #167 .
Lints pass:
Tests pass: