-
-
Notifications
You must be signed in to change notification settings - Fork 776
✨ Support bytes in Options and Arguments #1190
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
base: master
Are you sure you want to change the base?
✨ Support bytes in Options and Arguments #1190
Conversation
Add comprehensive tests for bytes type with base64 and hex encoding Add support for bytes type in Options and Arguments
Thanks for the PR, appreciate the time and effort spent on writing tests as well! We'll get this reviewed by the team and will get back to you 🙏 |
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.
@doubledare704, thank you for your interest and efforts!
As I see it, the main reason to introduce bytes
type support is to have the ability to use the same function outside the Typer, right?
Otherwise I don't see a point in this.. We can just accept str
and decode it in one line. That would even be easier in cases where we want to let users specify the encoding as a CLI option
This implementation assumes that the argument value is encoded with system default encoding. It doesn't allow to specify the encoding.
This implementation assumes that the argument value is encoded with the system default encoding. It doesn’t allow specifying the encoding.
I believe we should at least allow specifying the encoding as a parameter to Argument
/Option
.
But first, I would wait for Sebastian to approve the idea
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.
A few improvement opportunities:
Consider adding more edge case tests to further validate error handling.
In convert(), extracting decoding/encoding logic into helper functions could improve readability and reuse.
Documentation could be expanded with usage examples to make it easier for new users to try the features.
…ng-aware BytesParamType with helpers\n- Wire ParameterInfo.encoding/errors through Argument/Option\n- Add edge-case tests (non-ASCII, latin-1, ascii+errors, invalid encodings)\n- Expand examples to demonstrate encoding and errors\n- Add docs page: Tutorial -> CLI Parameter Types -> Bytes; wire into nav\n\nBackwards compatible: defaults to UTF-8 + 'strict' when not specified.
Summary of bytes support changes and request for approval Primary use case:
API and implementation:
Tests:
Docs/examples:
Questions:
Thanks! |
📝 Docs preview for commit 963b0d1 at: https://abb2fbfd.typertiangolo.pages.dev Modified Pages |
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
📝 Docs preview for commit b5111fc at: https://7acfb0c1.typertiangolo.pages.dev Modified Pages |
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
📝 Docs preview for commit a59129f at: https://08dc5e06.typertiangolo.pages.dev Modified Pages |
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
📝 Docs preview for commit 2e8b8ec at: https://047066fe.typertiangolo.pages.dev Modified Pages |
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
📝 Docs preview for commit 996a5d7 at: https://d330ec56.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 77c2846 at: https://5457df22.typertiangolo.pages.dev Modified Pages |
Attempt to solve #536