✨ Support bytes in Options and Arguments#1190
✨ Support bytes in Options and Arguments#1190doubledare704 wants to merge 22 commits intofastapi:masterfrom
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 🙏 |
YuriiMotov
left a comment
There was a problem hiding this comment.
@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
saky-semicolon
left a comment
There was a problem hiding this comment.
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! |
This comment was marked as outdated.
This comment was marked as outdated.
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
This comment was marked as outdated.
This comment was marked as outdated.
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
This comment was marked as outdated.
This comment was marked as outdated.
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
This comment was marked as outdated.
This comment was marked as outdated.
- Update test to assert error message in `stderr` instead of `stdout`. - Adjust indentation for function parameters to improve readability.
This comment was marked as outdated.
This comment was marked as outdated.
|
📝 Docs preview for commit 77c2846 at: https://5457df22.typertiangolo.pages.dev Modified Pages |
| @@ -0,0 +1,160 @@ | |||
| import base64 | |||
There was a problem hiding this comment.
As a quick comment, if we do decide to progress with this PR, we should add the correct tests for the tutorial files in test_tutorial/test_parameter_types/test_bytes.
There was a problem hiding this comment.
I've just added tests for tutorials. Please check.
| @@ -0,0 +1,38 @@ | |||
| # Bytes | |||
There was a problem hiding this comment.
This page should also be added to mkdocs.yml
There was a problem hiding this comment.
seems like already has a link in mkdocs
mkdocs.yml:118
- Add test cases for tutorials 001, 002, and 003 demonstrating bytes parameter usage. - Verify CLI behavior and script-level execution with coverage checks.
📝 Docs previewLast commit d86b0bd at: https://76abc87b.typertiangolo.pages.dev Modified Pages |
svlandeg
left a comment
There was a problem hiding this comment.
@doubledare704: as Yurii said, we'll wait for further reviews until Tiangolo approves the idea. We have got this on our internal board, so no need for further pings 🙏
Attempt to solve #536