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

fixed issue on multi select field while selecting single item #159

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

parthjoshi90
Copy link

Before this

  • It was required to pre-validate the values before the form was validated
    After this
  • Based on the annotation based on the types provided on the field it will parse the value.

fixes #122.

Copy link

codecov bot commented Jan 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (cec25c6) 94.05% compared to head (4062c67) 93.91%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   94.05%   93.91%   -0.14%     
==========================================
  Files          11       11              
  Lines         723      723              
==========================================
- Hits          680      679       -1     
- Misses         43       44       +1     

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

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for looking at this, we need to change approach and also need to add tests.

if len(values) == 1:
d[last_key] = values[0]
else:
if (last_key in model.model_fields and _t.get_origin(model.model_fields[last_key].annotation) is list) or len(values) > 1:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't going to work for lots of other types e.g. tuple, set, frozenset.

Instead of introspecting the python type, I'd rather we iterated over the JSON Schema and worked out which fields should take multiple values that way.

With that we can also:

  • move the logic out of unflatten thus separating concerns
  • cache the result per model
  • have a more general solution as JSON Schema already simplifies many types into an array schema

Since we already have logic to flatten JSON Schema, we might be able to use that and therefore pass a simple dict[key, bool] to this method.

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.

Bug with multi select fields
2 participants