-
-
Notifications
You must be signed in to change notification settings - Fork 672
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
🔥 Remove auto naming of groups added via add_typer
#1052
base: master
Are you sure you want to change the base?
Conversation
📝 Docs preview for commit 3e91df4 at: https://3eeb7be0.typertiangolo.pages.dev |
@svlandeg do you have any opinion on this? 😊 |
Great write-up and investigation, thank you @patrick91! 🙇 I think this would be fine, we would release it in a new 0.x version. And it would allow new features that are worth it. I'll still wait for @svlandeg's opinion in case she sees something obvious we aren't seeing yet. 😅 |
📝 Docs preview for commit f09b3c2 at: https://78985a46.typertiangolo.pages.dev |
📝 Docs preview for commit 6167f1d at: https://cb75dab2.typertiangolo.pages.dev |
📝 Docs preview for commit d55f9e9 at: https://afc212e1.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 08552ee at: https://89a0ff7f.typertiangolo.pages.dev Modified Pages |
@@ -21,6 +21,7 @@ def new_users(): | |||
app.add_typer(users_app, callback=new_users) | |||
|
|||
|
|||
# TODO: do we want this to work? | |||
@users_app.callback("call-users", help="Help from callback for users.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tiangolo I think we might want to remove support for this too 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think so.
📝 Docs preview for commit 3313794 at: https://8c9e9936.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit d1a59ee at: https://8c484d8f.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 2584f5e at: https://9042d7ba.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 8dcef2b at: https://dba42811.typertiangolo.pages.dev Modified Pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a detailed look into this and it does feel sensible to remove this behaviour. Great digging by Patrick to check the current usage of this in downstream applications, which appears to be pretty limited. We'd definitely have to warn users for the breaking change though.
It feels like it would make sense to also remove the help text taken from the callback, just for consistency.
I guess one final question I have is whether there's no other way to enable #1037, so we wouldn't have to touch existing behaviour.
### Name and help from callback parameter in `typer.Typer()` | ||
/// note | ||
|
||
In previous versions of Typer, in addition to the help text, the command name was also inferred from the callback function name, this is no longer the case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would specify the exact version of Typer in which this behaviour changed.
Thanks for the thorough review @svlandeg!
I think it's fine to keep support for the help from the callback, as it would only apply when there's an actual Click Group added instead of just the commands. And it would only be supported in the cases where the callback is also supported.
I was also thinking a bunch about this, but I think there wouldn't be a way, it was a design error from my side at the beginning, too few constraints on what is supported. 😅 Fortunately, it also didn't make much sense to many people as it's not too used. 😂 I think it's fine to remove it. We'll release a minor version with this (as Typer is pre-1.x that's equivalent to a major version) and add this in the breaking changes, with an example of how to upgrade. I think we are close to good to go. @patrick91 let me know when you're ready for me to give it a final review and merge. 🤓 |
📝 Docs preview for commit d45602e at: https://c9cd61b7.typertiangolo.pages.dev Modified Pages |
d45602e
to
53f65ba
Compare
📝 Docs preview for commit 53f65ba at: https://565ad54a.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit ff6a3ef at: https://e811c478.typertiangolo.pages.dev Modified Pages |
While working on #1037 (allowing
add_typer
to extend an app), we found that callbacks are sometimes used to implicitly get a group's name.This would happen in cases like this:
Output for the above
This is documented here: https://typer.tiangolo.com/tutorial/subcommands/name-and-help/#inferring-name-and-help-from-appcallback
With this PR, I'm proposing to remove this implicit behavior, this would make #1037 possible and without issue when using callbacks[1]
I did a quick code search and, while unfortunately GitHub only returns 5 pages of searches, it seems that almost no one is using this behaviour.
There's one interesting case, where they are pretty much using callbacks to do what #1037 should make it easier to implement:
https://github.com/pbdtools/xfds/blob/cf6f2a618ca8137a96eeb55eecadf6a61779d38c/src/xfds/_reset.py#L18-L19
And in cases where
name
isn't passed toadd_typer
, theTyper
instance has a name, like here:https://github.com/emergentmethods/flowdapt/blob/0be625437041b3b9c919f751d18969d7ce08c6b7/flowdapt/cli/db.py#L13-L18
[1] though they won't be supported in the first implementation