Skip to content
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

✨ feat: json and bytes field support in options #985

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

mhkarimi1383
Copy link

@mhkarimi1383 mhkarimi1383 commented Sep 9, 2024

Hi

Some times there is a need to have results as bytes (that is builtin supported by click.STRING)
and also there is a need of decoding entire value as an json object (useful when some params are too dynamic or getting that option in a prompt). I wanted to also add pydantic support (for both single option as json and all of the options in a single model) there and also use orjson (by checking if that's installed as an optional dependency), If that's Ok.

@svlandeg svlandeg added the feature New feature, enhancement or request label Sep 10, 2024
Copy link

📝 Docs preview for commit 137e1bc at: https://5f9b3d47.typertiangolo.pages.dev

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
Copy link

📝 Docs preview for commit 0f5a1b3 at: https://ae7752e5.typertiangolo.pages.dev

Copy link

📝 Docs preview for commit 432a48c at: https://b02f882e.typertiangolo.pages.dev

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
@mhkarimi1383 mhkarimi1383 changed the title ✨ feat: json and bytes field suppport in options ✨ feat: json and bytes field support in options Sep 10, 2024
Copy link

Signed-off-by: Muhammed Hussein Karimi <[email protected]>
Copy link

@mhkarimi1383
Copy link
Author

@svlandeg it's ok to ignore coverage on newly created class?

@svlandeg
Copy link
Member

No, in general we really need all the code in the main modules to be fully tested 🙏

Copy link

Copy link

@mhkarimi1383
Copy link
Author

mhkarimi1383 commented Sep 17, 2024

@svlandeg

I have added tests for newly created class

coverage is 100% again :)

@mhkarimi1383
Copy link
Author

If changes are Ok so far I can work on Pydantic support, Or move Pydantic support in another PR?

@svlandeg

@svlandeg
Copy link
Member

HI @mhkarimi1383!

We haven't yet been able to review this PR in detail. When we do, we'll update here. No need to ping individual maintainers in the meantime 😉

In terms of adding more functionality, it's always best to open separate PRs for distinct functionality. It makes the review process much easier if one PR deals with one "atomic" change.

Do have a look at existing PRs - if I recall correctly there's already a few PRs open with proposals for Pydantic support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature, enhancement or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants