Add a registry for configuration forms

Review Request #7827 — Created Dec. 24, 2015 and submitted

brennie
Djblets
release-0.10.x
7844, 7828
djblets

Review Board's "account pages", which are Djblets ConfigPages with
dynamically determined forms have been ported to Djblets. The behaviour
for dynamically configuring a form is encapsulated in the
DynamicConfigPageMixin and the ConfigPageRegistry. Config pages
that are to be dynamic should inherit from the DynamicConfigPageMixin
and are to be managed by a single registry; multiple registries can
manage multiple sets of configuration pages, however.

  • Ran unit tests.
  • Used this with an upcoming change for Review Board.
Description From Last Updated

All of our other errors in this form use a capital letter after the ":" (since it's often another exception's ...

chipx86chipx86

You can do: CONFIG_PAGE_REGISTRY_DEFAULT_ERRORS = dict({ FORM_ALREADY_REGISTERED: ..., ..., }, **DEFAULT_ERRORS)

chipx86chipx86

It'd be nice if this went into more detail as to where/how you'd use this.

chipx86chipx86

Blank line between these.

chipx86chipx86

Trailing whitespace.

daviddavid

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

This needs the full class path.

chipx86chipx86

Do we want to guard against registered_forms possibly being page_class.form_classes?

chipx86chipx86

Swap these.

chipx86chipx86

Typo on "djblets"

chipx86chipx86

No blank line.

chipx86chipx86

This should probably go into a little more detail on what the result is.

chipx86chipx86

Can't we just return if self._populated is False?

chipx86chipx86

Seems this would be a lot better as: for item in self._items.copy(): self.unregister(item)

chipx86chipx86

We should probably only call iter once.

daviddavid

Maybe a little more detail: """... Objects using this mixin are built to have a dynamic list of forms, which ...

chipx86chipx86

Two blank lines.

chipx86chipx86

I'd like to see more here about what this is for and how it's used.

chipx86chipx86

No need for this blank line.

chipx86chipx86

These should go up above the classes. Maybe put a comment above each saying what class they're for.

chipx86chipx86

No need for the blank line.

chipx86chipx86

Maybe write a bit here and with the remove function a to how this should be invoked. I assume callers ...

chipx86chipx86

"it" -> "the registry as".

chipx86chipx86
david
  1. 
      
  2. djblets/configforms/registry.py (Diff revision 1)
     
     

    Trailing whitespace.

  3. djblets/registries/registry.py (Diff revision 1)
     
     

    We should probably only call iter once.

    1. If we call iter only once, we will be modifying the object while iterating through it.

      next(iter(s)) for a set s is just getting an arbitrary element in the set so that we can remove it.

    2. The only other way I can see to do this is to make a list of self._items but that seems pretty inefficient.

  4. 
      
chipx86
  1. 
      
  2. djblets/configforms/registry.py (Diff revision 1)
     
     

    All of our other errors in this form use a capital letter after the ":" (since it's often another exception's error appearing after), so we should be consistent there.

    We also generally have the form of Could not register the form %(item)r: This form is already registered. Same sort of thing below.

  3. djblets/configforms/registry.py (Diff revision 1)
     
     
     
     
     
     
     
     

    You can do:

    CONFIG_PAGE_REGISTRY_DEFAULT_ERRORS = dict({
       FORM_ALREADY_REGISTERED: ...,
       ...,
    }, **DEFAULT_ERRORS)
    
    1. These are not equivalent.

  4. djblets/configforms/registry.py (Diff revision 1)
     
     

    It'd be nice if this went into more detail as to where/how you'd use this.

  5. djblets/configforms/registry.py (Diff revision 1)
     
     
     

    Blank line between these.

  6. djblets/configforms/registry.py (Diff revision 1)
     
     

    This needs the full class path.

  7. djblets/configforms/registry.py (Diff revision 1)
     
     
     
     
     

    Do we want to guard against registered_forms possibly being page_class.form_classes?

    1. Yes because we will be modifying the list while iterating through it.

  8. djblets/configforms/tests.py (Diff revision 1)
     
     
     

    Swap these.

  9. djblets/configforms/tests.py (Diff revision 1)
     
     

    Typo on "djblets"

  10. djblets/configforms/tests.py (Diff revision 1)
     
     
     
     

    No blank line.

  11. djblets/registries/registry.py (Diff revision 1)
     
     

    This should probably go into a little more detail on what the result is.

  12. djblets/registries/registry.py (Diff revision 1)
     
     

    Can't we just return if self._populated is False?

  13. djblets/registries/registry.py (Diff revision 1)
     
     
     
     

    Seems this would be a lot better as:

    for item in self._items.copy():
        self.unregister(item)
    
  14. 
      
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
  2. djblets/configforms/registry.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
  2. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
  2. 
      
chipx86
  1. 
      
  2. djblets/configforms/mixins.py (Diff revision 3)
     
     
     

    Maybe a little more detail:

    """...
    
    Objects using this mixin are built to have a dynamic list of forms,
    which can be controlled by callers. New forms can be registered on
    them, and existing forms unregistered. Through this, it's possible
    to extend a configuration page, allowing each form to load, validate,
    and save without the page hard-coding support for each form.
    
    Forms utilizing this mixin are expected to <whatever they're expected
    to do if using this mixin, such as setting a registry, and what that
    should be like, and how they should render it, etc.>
    """
    

    We should always assume in the docs that the user has no idea what this thing is for, why they'd use it, how they'd use it, or any details whatsoever about the implementation.

  3. djblets/configforms/registry.py (Diff revision 3)
     
     
     
     

    Two blank lines.

  4. djblets/configforms/registry.py (Diff revision 3)
     
     

    I'd like to see more here about what this is for and how it's used.

    1. This really isn't intended to be used externally. Only the ConfigPageRegistry should be.

  5. djblets/configforms/registry.py (Diff revision 3)
     
     
     
     

    No need for this blank line.

  6. djblets/configforms/registry.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These should go up above the classes. Maybe put a comment above each saying what class they're for.

  7. djblets/configforms/registry.py (Diff revision 3)
     
     
     
     

    No need for the blank line.

  8. djblets/configforms/registry.py (Diff revision 3)
     
     

    Maybe write a bit here and with the remove function a to how this should be invoked. I assume callers should only call this through the mixin, and not directly?

    1. The methods really only exist on the mixin because they existed on RB's config page stuff.

  9. djblets/registries/registry.py (Diff revision 3)
     
     

    "it" -> "the registry as".

  10. 
      
brennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        djblets/configforms/registry.py
        djblets/registries/registry.py
        djblets/configforms/mixins.py
        djblets/configforms/tests.py
    
    Ignored Files:
        docs/djblets/coderef/index.rst
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (67d3a0b)
Loading...