-
Notifications
You must be signed in to change notification settings - Fork 306
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
Support tuples in forms #52
Conversation
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.
Looks good, but you need to fix submission - at the moment data is being converted in unflatten
into something like this:
model_data: {
'profile_pic': <starlette.datastructures.UploadFile object at 0x1084ac090>,
'dob': '2023-12-01',
'size': {
'width': '123',
'height': '321',
},
'address': {
0: 'foobar',
1: 'x',
2: 'y',
},
In terms of solving this, I see two routes:
unflatten
taking the model and working out if the field is a tuple- assume that if all keys are completely numeric, then the dict should be converted to a list - this will probably be mucher easier
I think best to wait until we have #66 so you can add tests
I agree the second solution is simpler, but it appears I have an issue: This then gets complicated, because we need to check for each value if it's optional - and if so, fill it with a What do you think @samuelcolvin? Do I have to go for the second option? Or can the frontend somehow return |
Well, I guess the least bad option is to pass the model to It might be quickest (in terms of writing code at least, if not performance) to call our existing function to flatten the JSON Schema and get a dict of I hope that helps. |
Or we could just error and say it's not possible unless all fields are required as a first solution. |
I think that initial solution is much better for now. I think most use cases for tuples are gonna be something such as X, Y, Z and won't require optionals. I implemented this and updated the demo accordingly. |
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.
looking good, but needs tests now we have the machinery in place for tests.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 87.71% 94.01% +6.29%
==========================================
Files 11 11
Lines 700 718 +18
==========================================
+ Hits 614 675 +61
+ Misses 86 43 -43 ☔ View full report in Codecov by Sentry. |
Sorry I haven't been able to finish this myself and you had to step in @samuelcolvin, something popped up. Thank you for completing it! |
No problem, thanks for your work |
fixes #24