-
Notifications
You must be signed in to change notification settings - Fork 9
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
Change --text from argument to option #28
base: main
Are you sure you want to change the base?
Conversation
0ba12b3
to
4fa36b7
Compare
4fa36b7
to
77df172
Compare
This reverts commit c767a0a. This changes --text from typer.Argument to typer.Option This is a breaking change. It puts text on the same level as the other options like image or barcode. It is breaking because it now requires any text to be preceeded by `--text`. Ref: <labelle-org#12 (comment)> and below.
77df172
to
991be35
Compare
I still feel really conflicted about this. I know that this forces the user to be explicit about their attention, this avoids assumptions about a text element being essential, making it much less awkward to print just a QR or picture. But what if the primary use case is the occasional CLI user who 90% of the time just want to print a text label? I think one of the things $ dymoprint "hello world" Now the interaction will go something like $ labelle "hello world"
Usage: labelle [OPTIONS] COMMAND [ARGS]...
Try 'labelle --help' for help.
╭─ Error ───────────────────────────────────────────────────────────────────────╮
│ No such command 'hello world'. │
╰───────────────────────────────────────────────────────────────────────────────╯
$ labelle --help
Usage: labelle [OPTIONS] COMMAND [ARGS]...
╭─ Options ─────────────────────────────────────────────────────────────────────╮
│ --version Show application │
│ version │
│ --verbose -v Increase logging │
│ verbosity │
│ --sample-pattern INTEGER Prints test pattern of │
│ a desired dot width │
│ [default: None] │
...
╭─ Commands ────────────────────────────────────────────────────────────────────╮
│ list-devices │
╰───────────────────────────────────────────────────────────────────────────────╯ and now the user needs to read through the menu until they figure out that they need This may not seem dramatic, especially since we have our eyes so close to the code, and we are not the ones who would forget about I'm no UX expert, and I don't know what the right way forward here is. But I do think there is a logic to giving a CLI a default-to-text interface. I'm questioning if this change is prioritizing development convenience over interface simplicity. |
I don't really think we have many CLI users, and I think CLI is just a nice to have tool for sofisticated users. I think we should grow out of the concept that we do a nice utility to print simple text labels, and aim for a more complex tool, which allows us to design sofisticated labels, and which gives great GUI, web interface, libraries, etc.
So to summarize, I personally don't want to invest any more time on this, and I do want to have ability to provide actions other than just 'print' in the CLI. As long as there is no other way around this, I would still want to see this PR merged. Thought? |
I personally have the opposite impression based on recent interactions, e.g. #40 and this blog. But of course this is just speculation.
I think that would be a great solution. However, I don't immediately see any straightforward way of implementing this, since it seems like the argument parsing is handled deep within Click's internals. |
@tomers, I think I figured it out!!! 🎉 |
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.
Great progress! Thanks for maintaining the docs!
# Use standalone mode to avoid typer's default error handling here: | ||
# <https://github.com/tiangolo/typer/blob/773927208fbf03d30d50fc39fe2a1a18b7bd93cb/typer/core.py#L207-L216> | ||
# This allows us to add the following hint: | ||
add_hint_about_text_option(e) |
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 haven't test it, so I might be wrong here, but I think this function should return True
in case it found "No such command" and updated the message.
In case of True
, it should do the following two lines:
rich_format_error(e)
sys.exit(e.exit_code)
Otherwise, it should raise
.
This way we'll get proper error/exception handling by typer
, except for this specific 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.
The function only runs if there is an exception. In that case, it doesn't handle the exception, it simply mutates the message when necessary. Those two lines run in any case in order to handle the exception, so I don't see how returning a boolean would help. Does that make sense?
The goal is basically to handle the standalone_mode=False
branches from the code linked in the comment. Based on my own tests I think it's working well, but it'd be great if you could try if you can break something.
This is a breaking change. It puts text on the same level as the other options like image or barcode. It is breaking because it now requires any text to be preceeded by
--text
.Ref: #12 (comment) and below.
To be merged only before a major release.