-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Add support for fields with subwidgets (BoundWidget) #122
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #122 +/- ##
==========================================
- Coverage 89.85% 83.78% -6.08%
==========================================
Files 2 2
Lines 138 148 +10
Branches 48 49 +1
==========================================
Hits 124 124
- Misses 9 18 +9
- Partials 5 6 +1 ☔ View full report in Codecov by Sentry. |
here we are missing:
without that, we cannot accept or discuss your PR .. I will put your PR in the draft ... because is incomplete |
I'll get to it someday! |
Would love to see this merged. Quite an incomplete library without it. |
Any chance of tests and documentation for this? |
If someone has the opportunity to add the tests & docs for it I'd be happy to bake a new release :) Should be quite feasible to implement. |
I hoped to make some progress on it today, but I don't have a system that can run the tests, to begin with. It wants four different python versions installed! The linux hosts I have access to don't match up. And when running locally on MacOS, other things are messed up with tox, for me. Something about Long story short, somebody else is going to have to do this part. As for documentation, can you tell us what updates to the README are necessary? Beyond removing the part where it says this isn't supported. |
Hi @johncronan thanks for having a look at this again. I find the easiest way to have multiple versions of Python available on most systems is to use pyenv: https://github.com/pyenv/pyenv You could also use binary repos like "deadsnakes" for Ubuntu that have all the current versions available. After that you'll be able to install the versions required for testing and tox will take care of the rest. For tox itself I install it as a global tool using pipx but you can always install it inside a virtualenv if you want. For the documentation I'd say add a couple of examples of the new usage that is available, specifically how/if it differs from "regular" form fields. This could also help someone else to write the tests. |
I got something started for the documentation part. I don't remember what I was talking about, with the I recall that Django fields can have automatically-rendered labels, but in this context the "label" is the choice value, usually? All I really know is that, in order to get it to work, I had to do the labels part in my template code, not in Django. |
Looks good. Could we have a unit test that demonstrates the behaviour and covers the code path for the new logic? After that I think this would be ready for upstreaming? If you implement the unit test you don't have to run it with every Python version locally, one is enough. GitHub Actions will handle the full test matrix in CI :) |
Please merge. This functionality is long overdue. |
@alfonsrv if you fork and add unit tests for the feature then we can merge, at the current moment the feature is untested. |
It's quite a simple addition.
There is one disadvantage to this approach, which is that it assumes that the subfield will only be rendered with_label=False. I did this because it's a simple matter to attach a label using the template, and I'm not sure, but I think the implementation would otherwise have to be more complex.