Allow ConditionField to reflect dynamically-added condition choices.

Review Request #14279 — Created Dec. 18, 2024 and updated

Information

Djblets
release-5.x

Reviewers

The Conditions system was designed with the intent that conditions could
be dynamically registered and unregistered as needed, offering
extensibility in the types of condition choices presented and processed.

This was never tested until now, and we've found a few flaws. One of
those is that ConditionField was calculating the valid choices at
field definition time, and held on to that when copying for a new form
instance. The choices present initially would remain present, and we'd
never recalculate.

To work around this, we now defer any processing of choices until we
need them, and we always refer back to the ConditionSet provided at
the latest initialization time (such as when initializing fields for a
form instance).

We remember the initial choices provided to the field and to the widget,
and refer back to that whenever we need to start fresh. If this is a
class, it's instantiated as late as possible. Same if it's a callback
providing an instance. This state is never deep-copied over to a new
field, allowing new instances to reflect the available choices at that
moment in time.

ConditionsWidget is now the source of truth for the condition choices.
ConditionsField has a wrapper that forwards on to that one. This
simplifies lifecycles considerably.

To take full advantage of this, callers should pass in an instance and
not a class when caling the field.

There's also one fix in here for managing widgets. We were accessing
the value field's widget attribute, which didn't exist. Initially,
this code was built to work with form fields, but we moved to a wrapper
class. We now define a forwarding attribute to get the inner field's
widget.

Unit tests pass.

Tested this with other in-progress work for more dynamic condition
choices. Verified that I could enable an extension and see all the
choices.

Summary ID
Allow ConditionField to reflect dynamically-added condition choices.
The Conditions system was designed with the intent that conditions could be dynamically registered and unregistered as needed, offering extensibility in the types of condition choices presented and processed. This was never tested until now, and we've found a few flaws. One of those is that `ConditionField` was calculating the valid choices at field definition time, and held on to that when copying for a new form instance. The choices present initially would remain present, and we'd never recalculate. To work around this, we now defer any processing of choices until we need them, and we always refer back to the `ConditionSet` provided at the latest initialization time (such as when initializing fields for a form instance). We remember the initial choices provided to the field and to the widget, and refer back to that whenever we need to start fresh. If this is a class, it's instantiated as late as possible. Same if it's a callback providing an instance. This state is never deep-copied over to a new field, allowing new instances to reflect the available choices at that moment in time. `ConditionsWidget` is now the source of truth for the condition choices. `ConditionsField` has a wrapper that forwards on to that one. This simplifies lifecycles considerably. To take full advantage of this, callers should pass in an instance and not a class when caling the field.
de87e724fb503674a862a9891a5a386ecc4c2edb
Description From Last Updated

line too long (88 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

redefinition of unused 'Self' from line 11 Column: 1 Error code: F811

reviewbotreviewbot

More and more throughout the python ecosystem I'm seeing that setters are documented like regular methods ("Set the condition choices...") …

daviddavid

Can you add a module docstring?

daviddavid

Can you add some comments about what the purposes and difference between these two are?

maubinmaubin
There are no open issues
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

chipx86
david
  1. 
      
  2. djblets/forms/fields.py (Diff revision 2)
     
     
    Show all issues

    More and more throughout the python ecosystem I'm seeing that setters are documented like regular methods ("Set the condition choices...") while the property itself is documented like an attribute. You actually do that for ConditionsWidget.choices later in this change.

  3. Show all issues

    Can you add a module docstring?

  4. 
      
chipx86
maubin
  1. 
      
  2. djblets/forms/widgets.py (Diff revision 3)
     
     
     
    Show all issues

    Can you add some comments about what the purposes and difference between these two are?

  3. 
      
chipx86
Review request changed
Change Summary:

Updated the _orig_choices docs explaining the purpose for this attribute and how it differs from choices.

Commits:
Summary ID
Allow ConditionField to reflect dynamically-added condition choices.
The Conditions system was designed with the intent that conditions could be dynamically registered and unregistered as needed, offering extensibility in the types of condition choices presented and processed. This was never tested until now, and we've found a few flaws. One of those is that `ConditionField` was calculating the valid choices at field definition time, and held on to that when copying for a new form instance. The choices present initially would remain present, and we'd never recalculate. To work around this, we now defer any processing of choices until we need them, and we always refer back to the `ConditionSet` provided at the latest initialization time (such as when initializing fields for a form instance). We remember the initial choices provided to the field and to the widget, and refer back to that whenever we need to start fresh. If this is a class, it's instantiated as late as possible. Same if it's a callback providing an instance. This state is never deep-copied over to a new field, allowing new instances to reflect the available choices at that moment in time. `ConditionsWidget` is now the source of truth for the condition choices. `ConditionsField` has a wrapper that forwards on to that one. This simplifies lifecycles considerably. To take full advantage of this, callers should pass in an instance and not a class when caling the field.
8c532363c6245a38c67ca82321287f03ca5a960d
Allow ConditionField to reflect dynamically-added condition choices.
The Conditions system was designed with the intent that conditions could be dynamically registered and unregistered as needed, offering extensibility in the types of condition choices presented and processed. This was never tested until now, and we've found a few flaws. One of those is that `ConditionField` was calculating the valid choices at field definition time, and held on to that when copying for a new form instance. The choices present initially would remain present, and we'd never recalculate. To work around this, we now defer any processing of choices until we need them, and we always refer back to the `ConditionSet` provided at the latest initialization time (such as when initializing fields for a form instance). We remember the initial choices provided to the field and to the widget, and refer back to that whenever we need to start fresh. If this is a class, it's instantiated as late as possible. Same if it's a callback providing an instance. This state is never deep-copied over to a new field, allowing new instances to reflect the available choices at that moment in time. `ConditionsWidget` is now the source of truth for the condition choices. `ConditionsField` has a wrapper that forwards on to that one. This simplifies lifecycles considerably. To take full advantage of this, callers should pass in an instance and not a class when caling the field.
de87e724fb503674a862a9891a5a386ecc4c2edb
Diff:

Revision 4 (+572 -112)

Show changes

djblets/conditions/values.py
djblets/forms/fields.py
djblets/forms/widgets.py
djblets/forms/tests/test_conditions_field.py
djblets/forms/tests/test_conditions_widget.py

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
maubin
  1. Ship It!
  2. 
      
david
  1. Ship It!
  2. 
      
Loading...