Modify djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
Review Request #11858 — Created Oct. 20, 2021 and submitted
Previously, ListEditWidget expects values to be strings with a valid
separator, thus it can only be used to edit a list of strings.
The corresponding Django template and Backbone view are hardcoded to
display string values with text inputs.This change modifies ListEditWidget so that it can modify a list of
any type of values, not just strings. This change includes the creation
of a ListEditField which has the ListEditWidget as its default widget.
This field splits a string into a list of strings, reproducing the
behaviour of the old ListEditWidget. We also introduce a
ListEditDictionaryField, which takes a dictionary and splits it into a
list of (key, value) tuples.
- Wrote tests for ListEditField and ListEditDictionaryField. Ran all
tests indjblets.forms.tests
with success. - Updated tests for ListEditWidget and ListEditView. Ran all Javascript Unit
tests with success. - Performed manual tests.
Summary | ID | Author |
---|---|---|
b1e246ac22d4e14e8c5a2e0ae6832f07f8a40c0b | Michelle |
Description | From | Last Updated |
---|---|---|
Typo in summary: dbjlets -> djblets |
david | |
Please wrap the description/testing done to 70 columns. |
david | |
Can we change the summary to be "Modify" instead of "Modifying"? |
david | |
It would be nice to write the testing done as a set of bullet points. Right now it's kind of … |
david | |
W293 blank line contains whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
Will change {{forloop.counter0}} to 0 |
maubin | |
This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a .es6.js … |
chipx86 | |
Django's MultiWidget also takes in a child widget (well, a list of them). It accepts either a class or an … |
chipx86 | |
So here's a big thing I missed in my prior review (and this is all from the original design, which … |
chipx86 | |
This could be one statement. |
chipx86 | |
No need for parens around name. In fact, (name) is equivalent to name. If we instead had (name,), then it'd … |
chipx86 | |
I suspect we won't want to use attrs itself here. Those are the attributes for the ListEditWidget, which will probably … |
chipx86 | |
We reference self.value_widget many times. Each time we do that, we have to go through the attribute accessor paths in … |
chipx86 | |
Missing a trailing comma. |
chipx86 | |
Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, … |
chipx86 | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
W391 blank line at end of file |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
E231 missing whitespace after ',' |
reviewbot | |
W292 no newline at end of file |
reviewbot | |
It's possible that when you are creating ListEditView here, it is not defined, That's why the error Cannot read properties … |
akim.ruslanov | |
You're actually storing this as _sep in the __init__ method. We don't need to have any documentation for purely internal/private … |
david | |
This can be dedented 4 spaces. |
david | |
Dedent 4 spaces. |
david | |
This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first … |
david | |
Because this init method doesn't do anything but call the superclass, we can just get rid of it. |
david | |
Dedent 4 spaces. |
david | |
Dedent 4 spaces. |
david | |
This should have a "Raises" section with django.forms.ValidationError |
david | |
This new line can go away (plus you added some trailing whitespace). |
david | |
This new line can go away (plus you added some trailing whitespace). |
david | |
Please add a module docstring for this. |
david | |
Please add a module docstring for this. |
david | |
We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else. |
david | |
Add a blank line between these two. For properties, we can also skip "Returns" and just put in: Type: django.forms.widgets.Media … |
david | |
We do sometimes say "list or unicode" to clarify here. |
david | |
Trailing whitespace on this line. |
david | |
Trailing whitespace on this line. |
david | |
Col: 87 Missing semicolon. |
reviewbot | |
Trailing whitespace on this line. |
david | |
Trailing whitespace on this line. |
david | |
Trailing whitespace on this line. |
david | |
Trailing whitespace on this line. |
david | |
We don't use var in .es6 files. That said, if you change the .each() to use a fat-arrow function, you … |
david | |
Col: 62 Missing semicolon. |
reviewbot | |
Col: 7 Missing semicolon. |
reviewbot | |
Please keep the import list alphabetized |
david | |
Add a blank line between these two (in python classes, there's one blank line between each thing, and the docstring … |
david | |
These are each indented one space too many. |
david | |
Each of these lines needs to be indented one more space. |
david | |
This new line can be removed. |
david | |
Add a blank line between these two. |
david | |
Add a blank line between these two. |
david | |
f before w |
david | |
Test docstrings shouldn't have a period at the end, because the test runner will print an ellipsis. |
david | |
Type definitions don't need an explanation, just the type name. The explanation of the property is in the docstring above … |
david | |
Add a blank line between these two. |
david | |
This line can go away. |
david | |
Instead of passing in len to filter, we should use None. This will do a falsy check instead of actually … |
david | |
Type here can be jQuery |
david | |
It looks like this is taking in a jQuery, so we should list that here and call the argument name … |
david | |
Let's define a variable for $(el) so we don't have to construct the jQuery object twice. |
david | |
Should this be removed? |
david | |
We no longer need this on djblets release-3.x or master. |
david | |
We no longer need this on djblets release-3.x or master. |
david | |
We need to add some blank lines between all of these. |
david | |
The outer parens aren't necessary here. |
david |
- Description:
-
~ Previously, ListEditWidget expects values to be strings.
~ Initial modifications to list edit widget ~ Previously, ListEditWidget expects values to be strings rendered with a TextInput widget.
~ Initial modifications to list edit widget so that values can be rendered by any widget - Testing Done:
-
~ Tested on dev server, add button does not work
~ Testing on dev server, "Add Item" button saves the entire form instead of just adding another entry to the list widget (not sure why)
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle
Checks run (2 succeeded)
-
-
This trailing comma will break older browsers, since it's outputted directly to the browser, and is not in a
.es6.js
file (which would compile suitably for the browsers we need to support). -
Django's
MultiWidget
also takes in a child widget (well, a list of them). It accepts either a class or an instance in the constructor, and if it's a class, it'll instantiate it.I think that's the right pattern, and the right default. Taking in a class just stores a reference to the class in the list of default values for the function.
Making the default an instance means that:
- We instantiate the
TextInput
and keep this around forever (using memory that may not be needed) - We actually share that default instance across all
ListEditWidget
instances by default, which could lead to errors with state being clobbered.
Let's make the default a reference to the class, and then in the constructor we can instantiate it if it's a class or store directly if the caller provided an instance.
- We instantiate the
-
-
No need for parens around
name
. In fact,(name)
is equivalent toname
. If we instead had(name,)
, then it'd be a tuple with one value (name
) in it.A tuple is often a correct thing to pass in using
%
-based formatting, but isn't necessary in this particular case.Caveat: If the value being passed in was iterable (a list or tuple, for example), we would need to wrap it as
(value,)
, or the%
-formatting would be looking inside of it for values to stuff into the%s
and such. -
I suspect we won't want to use
attrs
itself here. Those are the attributes for theListEditWidget
, which will probably differ.A base set of
attrs
are generally provided upon widget instantiation (or overridden in the widget itself), and then built upon when it's time to render. So starting out, we'd probably want:widget_attrs = value_widget.attrs
Which we could pass in to
render()
.If we wanted to add our own attributes to each value (say, giving them each a unique ID in the DOM — something worth considering in the future — consider the following an example only), we'd do something like:
common_widget_attrs = value_widget.attrs for i, val in enumerate(value_list): widget_attrs = {} if 'id' in attrs: widget_attrs['id'] = '%s_%s' % (self.id_for_label(attrs['id']), i) render( ..., attrs=value_widget.build_attrs(common_widget_attrs, widget_attrs))
So, taking those base attributes, and then modifying them on each loop so it's suitable for that row.
For reference:
enumerate
wraps an iterable, yielding an index and the value for each iteration.id_for_label
returns a suitable ID for the widget.build_attrs
takes in common attributes and specific attributes and returns a new dictionary combining the two.
-
We reference
self.value_widget
many times. Each time we do that, we have to go through the attribute accessor paths in the class. Not particularly slow, but not worth doing. Let's pull outvalue_widget
into a local variable before the loop above, so accesses are fast. -
-
Technically, this makes this a breaking change, which requires a Djblets major version bump (we follow semantic versioning in Djblets, at least nowadays).
This is probably fine, since we're targeting Djblets 3.0 for this work, but we need to document this with the following before the
Args
section:Version Changed: 3.0: * Removed the `inputAttrs` option. * Added the `renderedDefaultRow` option.
(Also,
renderedDefaultRow
will need to be documented.)
-
-
So here's a big thing I missed in my prior review (and this is all from the original design, which makes modernizing this tricky).
This code is still assuming that we're dealing strictly with individual strings (and that we can just split them). That isn't safe, as the widget may be returning an integer, a list, a dictionary, or even string-encoded content that may get split wrong.
However, we don't want to break existing consumers.
The problem comes down to the original design. This assumed it was only ever going to work with strings, and that the data being fed in was a string that had a valid separator.
It should have been designed to work in conjunction with a matching field that prepared the value before it got to
render()
, turning whatever the content would be into a list of values that pertained to the widgets.Basically, it should be the job of the field to prepare data and the job of a widget to render it. The widget shouldn't have to be parsing anything.
So I think we can work our way toward this design by doing the following:
- In
render()
, check ifvalue
is a list. If so, we're good. This is modern. If not, convert it to a list through this existing logic we have here. (Update the code to reusevalue
, instead ofvalue_list
, to make it clear that this is what we expect of it.) - Add a comment saying that we plan to deprecate this behavior. In a future release, we'll enable deprecation warnings for this code path.
- Introduce a
djblets.forms.fields.ListEditField
. This will be the new field intended to work withListEditWidget
. Its job will be to take in a string and convert it to values, and to associateListEditWidget
as the default widget. Subclasses (such as your field for the Pygments work) can do that conversion however is needed. By default, it should split in the same way that the widget otherwise would.
This effectively gives us a migration path to that ideal world in which the widget just renders, and the field handles the data processing. It would then be really easy to subclass these and get the data and widget setup in the right form.
That may sound like a lot, but the field work isn't going to be too bad. Not documented as well as it should be, but I can guide you through any of it.
- In
- Change Summary:
-
Create ListEditField and ListEditDictionaryField and their tests
- Description:
-
~ Previously, ListEditWidget expects values to be strings rendered with a TextInput widget.
~ Initial modifications to list edit widget so that values can be rendered by any widget ~ Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.
~ + This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.
+ + TODO:
+ - ModifyListEditWidget and its template and backbone view (and tests for each of those) - Testing Done:
-
~ Testing on dev server, "Add Item" button saves the entire form instead of just adding another entry to the list widget (not sure why)
~ Added tests for ListEditField and ListEditDictionaryField, ran successfully
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle - Diff:
-
Revision 3 (+539 -63)
Checks run (1 failed, 1 succeeded)
flake8
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle - Diff:
-
Revision 4 (+555 -81)
Checks run (2 succeeded)
- Change Summary:
-
Add get_context and value_from_datadict methods
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle - Diff:
-
Revision 5 (+744 -144)
Checks run (2 succeeded)
- Change Summary:
-
Working on javascript logic for dynamically adding and removing values in listeditwidget.
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle - Diff:
-
Revision 6 (+812 -238)
Checks run (2 succeeded)
-
-
It's possible that when you are creating ListEditView here, it is not defined, That's why the error
Cannot read properties of undefined (reading 'ListEditView')
happens? I would try to see if I can do something likeconsole.log(Djblets.Forms)
just to see if the class is properly imported/accessible within this HTML. You might need to add extra code to specify where to get the ListEditView from
-
-
Are you calling/importing the Form correctly? Adding the correct classes (ex. multipart/form) when rendering it in the HTML? Does new items in the listeditview require a different form of parsing than just parsing the text?
I would also console.log() it out to see if the result is what you're expecting, and check the types of the results too.
- Change Summary:
-
Done all the logic for ListEditWidget, need to modify tests
- Description:
-
Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.
~ This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change also includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.
~ This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.
TODO:
~ - ModifyListEditWidget and its template and backbone view (and tests for each of those) ~ - Modify tests for ListEditWidget, its template and backbone view - Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle - Diff:
-
Revision 7 (+974 -326)
-
I know this is still marked WIP, but it's looking really solid so I did a pretty thorough review.
-
-
-
You're actually storing this as
_sep
in the__init__
method. We don't need to have any documentation for purely internal/private attributes, unless they're particularly confusing or complicated. -
-
-
This should just be indented 4 spaces, not 7. Even better would be to reformat the docstring so the first line fits on one line and move any details into a second paragraph.
-
-
-
-
-
-
-
-
-
We generally avoid the python ternary syntax because it's not particularly readable. Let's just make this an if/else.
-
Add a blank line between these two.
For properties, we can also skip "Returns" and just put in:
Type: django.forms.widgets.Media
because callers can use it as if it were just a plain attribute and not a method.
-
-
-
-
-
-
-
-
We don't use
var
in .es6 files. That said, if you change the.each()
to use a fat-arrow function, you can just usethis
directly instead of aliasing it toself
.
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle 47621a8995c54c783c375169d32fd0d20fe12b8d Michelle - Diff:
-
Revision 8 (+1027 -375)
Checks run (2 succeeded)
- Summary:
-
[WIP] Modifying dbjlets.forms.widgets.ListEditWidget to handle any type of widget and form field[WIP] Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
- Description:
-
~ Previously, ListEditWidget expects values to be strings with a valid separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs.
~ Previously, ListEditWidget expects values to be strings with a valid
+ separator, thus it can only be used to edit a list of strings. + The corresponding Django template and Backbone view are hardcoded to + display string values with text inputs. ~ This change modifies ListEditWidget so that it can modify a list of any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples.
~ This change modifies ListEditWidget so that it can modify a list of
+ any type of values, not just strings. This change includes the creation + of a ListEditField which has the ListEditWidget as its default widget. + This field splits a string into a list of strings, reproducing the + behaviour of the old ListEditWidget. We also introduce a + ListEditDictionaryField, which takes a dictionary and splits it into a + list of (key, value) tuples. TODO:
- Modify tests for ListEditWidget, its template and backbone view - Testing Done:
-
~ Added tests for ListEditField and ListEditDictionaryField, ran successfully
~ Added tests for ListEditField and ListEditDictionaryField.
- Summary:
-
[WIP] Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form fieldModifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
- Description:
-
Previously, ListEditWidget expects values to be strings with a valid
separator, thus it can only be used to edit a list of strings. The corresponding Django template and Backbone view are hardcoded to display string values with text inputs. This change modifies ListEditWidget so that it can modify a list of
any type of values, not just strings. This change includes the creation of a ListEditField which has the ListEditWidget as its default widget. This field splits a string into a list of strings, reproducing the behaviour of the old ListEditWidget. We also introduce a ListEditDictionaryField, which takes a dictionary and splits it into a list of (key, value) tuples. - - TODO:
- - Modify tests for ListEditWidget, its template and backbone view - Testing Done:
-
~ Added tests for ListEditField and ListEditDictionaryField.
~ Added tests for ListEditField and ListEditDictionaryField. Ran all
+ tests in djblets.forms.tests with success. + Updated tests for ListEditWidget and ListEditView. Ran all Javasccript + Unit tests with success (2 fail but they also fail on the master + branch) - Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle 47621a8995c54c783c375169d32fd0d20fe12b8d Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle f95bcbc519ddd5ec851e88ce2e69989a3028c0dc Michelle - Diff:
-
Revision 9 (+1605 -623)
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle f95bcbc519ddd5ec851e88ce2e69989a3028c0dc Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle 31e17c1f3f5a05dd41b3d965526af549827a6ae2 Michelle - Diff:
-
Revision 10 (+1601 -619)
Checks run (2 succeeded)
-
-
-
Add a blank line between these two (in python classes, there's one blank line between each thing, and the docstring counts as a thing).
-
-
-
-
-
-
-
-
Type definitions don't need an explanation, just the type name. The explanation of the property is in the docstring above this.
-
-
-
Instead of passing in
len
to filter, we should useNone
. This will do a falsy check instead of actually checking the length of the string, which is ever so slightly faster. -
-
It looks like this is taking in a jQuery, so we should list that here and call the argument name $entry
-
-
- Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle 31e17c1f3f5a05dd41b3d965526af549827a6ae2 Michelle 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle 31e17c1f3f5a05dd41b3d965526af549827a6ae2 Michelle b1ac23a933599e054d62d871034eee7b0be4127e Michelle - Diff:
-
Revision 11 (+1626 -640)
Checks run (2 succeeded)
- Testing Done:
-
Added tests for ListEditField and ListEditDictionaryField. Ran all
tests in djblets.forms.tests with success. ~ Updated tests for ListEditWidget and ListEditView. Ran all Javasccript ~ Unit tests with success (2 fail but they also fail on the master ~ branch) ~ Updated tests for ListEditWidget and ListEditView. Ran all Javasccript Unit ~ tests with success (tests related to ConditionSet and ~ ConditionValueFormFieldView fail but they also fail on the release-3.x and + reviewboard release-5.0.x branches). + Also performed manual tests. - Commits:
-
Summary ID Author 7c5aea1b0b7622ad5965b72c7e0b9f649eef63eb Michelle 7ed90eb1f579fd4b2d96566aef69dbab98424ff1 Michelle 8c5a5e698f733d5e974367f9c85d2f436353873c Michelle 8d377e93fc24ccd29beac106c037d4facd825b2e Michelle 2df3771437170564840a4c768e49bedca3881606 Michelle 6a3248027c7207c6eab7fa6a9d4fccf51d1a9a91 Michelle aa46cef736df948002f411c0457144bc224102c5 Michelle 530d3a8a4a14d1ca488fbfb2cee37ea69fc59713 Michelle c61d3014fab76685f6e13b2bcdc1bcbd3ef52c44 Michelle fbafa58ad53cdbd50741ad5989e5724f8b989f74 Michelle 715daf675db9d544ef76bf432b3b7f81c5431b62 Michelle bf85d33b56b87f2f5186fe4c5508f78126d81bf6 Michelle 4e8cc1c71e3c4435878036373b7900d0cc70ffa1 Michelle cb83b214f746bcff67214a847e213091490b03b1 Michelle 31e17c1f3f5a05dd41b3d965526af549827a6ae2 Michelle b1ac23a933599e054d62d871034eee7b0be4127e Michelle 9e21d68a1b619b24677805db8628a8b388c45d98 Michelle - Branch:
-
masterrelease-3.x
- Diff:
-
Revision 12 (+1400 -420)
Checks run (2 succeeded)
- Summary:
-
Modifying djblets.forms.widgets.ListEditWidget to handle any type of widget and form fieldModify djblets.forms.widgets.ListEditWidget to handle any type of widget and form field
- Testing Done:
-
~ Added tests for ListEditField and ListEditDictionaryField. Ran all
~ tests in djblets.forms.tests with success. ~ Updated tests for ListEditWidget and ListEditView. Ran all Javasccript Unit ~ - Wrote tests for ListEditField and ListEditDictionaryField. Ran all
tests indjblets.forms.tests
with success.
~ - Updated tests for ListEditWidget and ListEditView. Ran all Javasccript Unit
tests with success.
~ - Performed manual tests.
- tests with success (tests related to ConditionSet and - ConditionValueFormFieldView fail but they also fail on the release-3.x and - reviewboard release-5.0.x branches). - Also performed manual tests. - Wrote tests for ListEditField and ListEditDictionaryField. Ran all
- Commits:
-
Summary ID Author 9e21d68a1b619b24677805db8628a8b388c45d98 Michelle 195d3f4f3cdcfeeea72757443c19e38c6488738a Michelle - Diff:
-
Revision 13 (+1398 -420)
Checks run (2 succeeded)
- Testing Done:
-
- Wrote tests for ListEditField and ListEditDictionaryField. Ran all
tests indjblets.forms.tests
with success.
~ - Updated tests for ListEditWidget and ListEditView. Ran all Javasccript Unit
tests with success.
~ - Updated tests for ListEditWidget and ListEditView. Ran all Javascript Unit
tests with success.
- Performed manual tests.
- Wrote tests for ListEditField and ListEditDictionaryField. Ran all
- Commits:
-
Summary ID Author 195d3f4f3cdcfeeea72757443c19e38c6488738a Michelle b31f9b2f63f824083e6576f033d5982390cb31d8 Michelle - Diff:
-
Revision 14 (+1400 -420)
Checks run (2 succeeded)
- Commits:
-
Summary ID Author b31f9b2f63f824083e6576f033d5982390cb31d8 Michelle dd228ad79d5342cffbc6e385143582fe2af0730a Michelle - Diff:
-
Revision 15 (+1492 -428)
Checks run (2 succeeded)
- Change Summary:
-
got rid of some unnecessary "Version Added:"s
- Commits:
-
Summary ID Author dd228ad79d5342cffbc6e385143582fe2af0730a Michelle b1e246ac22d4e14e8c5a2e0ae6832f07f8a40c0b Michelle - Diff:
-
Revision 16 (+1462 -428)