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

Parse json data from token if object is dict-like. #285

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

BrianPugh
Copy link
Owner

@BrianPugh BrianPugh commented Jan 3, 2025

If an object is dict/dataclass-like, attempts to parse input data as a json-string.

@AngellusMortis can you play around with this and see if it satisfies your needs? I need to flesh out the rest of the PR and think a little harder if this is the appropriate mechanism, but it naively seems to work.

Addresses #282.

TODO

  • unit tests
  • docs

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.97%. Comparing base (6284a12) to head (4aa51b9).

Files with missing lines Patch % Lines
cyclopts/argument.py 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
- Coverage   94.00%   93.97%   -0.04%     
==========================================
  Files          26       26              
  Lines        3020     3035      +15     
  Branches      636      640       +4     
==========================================
+ Hits         2839     2852      +13     
- Misses         88       90       +2     
  Partials       93       93              
Flag Coverage Δ
unittests 93.97% <87.50%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AngellusMortis
Copy link

AngellusMortis commented Jan 3, 2025

It looks like data is getting populated correctly, but the correct values are not being passed to Pydantic correctly (I am using pydantic). It looks like if it was just a basic dataclass, it would work. (lines at the top are manual prints I added in)

image

@AngellusMortis
Copy link

That commit looks good!

@BrianPugh
Copy link
Owner Author

@AngellusMortis I just pushed a fix for that; please give it a try! Pydantic is a more-special-than-usual case within cyclopts; as it's the only type that cyclopts practically disables it's own coercion engine for.

@BrianPugh
Copy link
Owner Author

that was a lightning fast reply!

@AngellusMortis
Copy link

Watching a show right now. So Github is up and just automatically updating for me. lol.

@BrianPugh
Copy link
Owner Author

I'd appreciate it if you'd keep playing with it and let me know if you discover any other issues. Otherwise tonight/this weekend I'll be adding tests/docs for this new feature! Thanks!

@AngellusMortis
Copy link

AngellusMortis commented Jan 4, 2025

Find an issue: If you have aliases for your pydantic class, it does not accept the fields from the alias value.

i.e. If you have a config that converts camelCase to snake_case:

class BaseK8sModel(BaseModel):
    """Base model for k8s spec."""

    model_config = ConfigDict(
        alias_generator=to_camel,
        populate_by_name=True,
        from_attributes=True,
    )

It cannot load the values from camelCase, only snake_case.

I did confirm that passing both the ENV and one of the nested properties via an option still works.

@BrianPugh
Copy link
Owner Author

I'll look into this, thanks!

@AngellusMortis
Copy link

AngellusMortis commented Jan 11, 2025

@BrianPugh Can you rebase this branch off of main (to get the fix for the nested meta apps)? Thanks!

@BrianPugh
Copy link
Owner Author

rebased! I still want to rethink this a little more; because as of right now it sort of bypasses a lot of Cyclopts mechanisms. It probably works well for most general use-cases (once I investigate the pydantic issue you brought up), but would like it to work for even those advanced use-cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants