How should the writer= override work?

[jean]

I wanted to add the following test:

If ``persist_writer`` is provided by the widget, passing ``writer`` overrides it::

    >>> form['yet_another_field'] = factory(
    ...     'text',
    ...     props={
    ...         'persist': True,
    ...         'persist_writer': write_mapping_writer
    ...     })
    >>> data = form.extract(request={
    ...     'form.yet_another_field': 'value',
    ... })
    >>> model = MixedModel()
    >>> data['yet_another_field'].write(model, writer=attribute_writer)
    >>> model.yet_another_field
    'value'

But this fails:

File "/home/jean/repos/git/yafowil/src/yafowil/persistence.rst", line 182, in persistence.rst
Failed example:
    model.yet_another_field
Exception raised:
    Traceback (most recent call last):
      File "/usr/lib/python2.7/doctest.py", line 1315, in __run
        compileflags, 1) in test.globs
      File "<doctest persistence.rst[63]>", line 1, in <module>
        model.yet_another_field
    AttributeError: 'MixedModel' object has no attribute 'yet_another_field'

If I understand correctly, it works like this:

• if the form has self.persist_writer, that is used throughout.
• if writer= is passed, that is used for child nodes that don't have self.persist_writer set.
• but writer= is not used for nodes that already have self.persist_writer set.

That seems surprising. I would expect writer= to always govern if it's passed. So I think I'm probably misunderstanding. If you have some time, could you let me know if I am?

[rnixx]

The MixedModel test class inherits from dict and defines one class attribute my_other_field. What you're trying to do is to write yet_another_field as attribute to the model due to the used attribute_writer. That's why it fails.

The default writer callbacks behave as follows:

• attribute_writer writes the extracted value to model.fieldname
• write_mapping_writer writes the extracted value to model['fieldname']
• node_attribute_writer writes the extracted value to model.attrs['filedname']

The last one is meant for Applications dealing with node based model implementations.

[rnixx]

Normally you don't call data['yet_another_field'].write in real world forms. As you properly did is to define 'persist_writer': write_mapping_writer in widget props for yet_another_field. The write function gets called recursively by default (you can prevent this if desired) and the proper persist callback is passed to it - that means the default writer if not overwritten on child widget or the dedicated writer from widget props.

[rnixx]

Ah, now I think I got you.

The reason why passed writer on child widget does not take precedence over self.persist_writer is that default writer still takes effect on subsequent child widgets instead of the dedicated one, which is actually fine. But sure, that's confusing if some tries to do what you do. So I think we should pass the default writer as dedicated keyword argument and then it's possible to implement what you want.

But just to say it again, the writer should be defined in the widget definition

[jean]

Hi @rnixx :-) What would you think of (always used passed-in writer):

     def write(self, model, writer=None, recursiv=True):
         if self.has_errors:
             raise RuntimeError(
                 'Attempt to persist data which failed to extract'
             )
-        current_writer = self.persist_writer or writer
+        current_writer = writer or self.persist_writer
         if not current_writer:
             raise ValueError('No persistence writer found')
-        if not writer:
-            writer = current_writer
         if self.persist:
             target = self.persist_target
             if not target:
@@ -141,7 +140,7 @@ class RuntimeData(object):
         if not recursiv:
             return
         for child in self.values():
-            child.write(model, writer=writer, recursiv=recursiv)
+            child.write(model, writer=current_writer, recursiv=recursiv)

[rnixx]

I'd prefer the following to keep the default writer passing from parent to subsequent widgets sane:

def write(self, model, default_writer=None, writer=None, recursiv=True):
    ...
    # and the writer lookup
    current_writer = writer or self.persist_writer or default_writer
    ...
    for child in self.values():
        child.write(model, default_writer=default_writer, recursiv=recursiv)

This keeps the following situation sane:

form (defaut writer)
    compound (custom writer)
        child (default writer)

When you call:

form.write(model)

[jean]

Hmm. I still don't really like this. This implies:

• writer= is only for the immediate widget, not for any child widgets.
• default_writer= only applies if a child widget doesn't define self.persist_writer (i.e. child widgets' writer can't be overridden).

If possible, I prefer simply: always use writer= if it's passed. If recursiv=True: use it for everything in the tree.

Use cases:

• write a tree of widgets, use writer= only for widgets that don't already define self.persist_writer (no overrides). (Child widgets use their own writer if defined, or the original writer= if passed, or root's writer.) This is current behaviour.
• write a tree of widgets, use writer= for root (override) and for any child widgets that don't already define self.persist_writer (no recursive overrides). (Child widgets use their own writer if defined, or writer= if passed.) This is a less confusing alternative to current behaviour.
• write a tree of widgets, always use writer= if passed in. This is what I'm proposing.
• write a tree of widgets, use writer= to override only the root writer, and use default_writer= only for child widgets that don't already define self.persist_writer (no recursive overrides). (Don't pass on root's writer.) This is what you're proposing.

The current behaviour basically changes the default for "blank" widgets from root's writer to writer=. Why would you want to keep root's writer and have only "blank" child widgets use another writer?

The only one that I find intuitive is my proposal, the others are surprising: why pass a parameter if it's going to be ignored by widgets that have their own opinion? In this case, if you actually want your specified writer to be used, you have to form[child][child].attrs['persist_writer'] = mywriter for all opinionated widgets.

Sorry for belabouring this.

[jean]

The MixedModel test class inherits from dict and defines one class attribute my_other_field.

Ah, of course, duh :-)
If I fix that the error is:

File "/home/jean/repos/git/yafowil/src/yafowil/persistence.rst", line 184, in persistence.rst
Failed example:
    model.yet_another_field
Expected:
    'value'
Got:
    <UNSET>

[jean]

But just to say it again, the writer should be defined in the widget definition

I assumed that the passed-in writer is an override, but evidently it's meant more as a fallback.
I'm OK with that as well, mainly I'm just trying to understand how it's supposed to work.

[rnixx]

@jean Your objections are valid and I'm happy that you point it out. Things can only get better if they are discussed ;)

Actually fallback_writer would be a better name as writer in the current implementation. The base idea was always that writer is not given directly to write but be handled transparently to the API user.

In common forms it's normally totally adequate to just define the persist_writer on root widget. Common case is you want the results of the extracted widgets being written to instance attributes or via __setitem__.

A case where it might be worth for a custom writer somewhere in the tree is if some data should not end up on the given model, or additionally should end up elsewhere. Such writers might be defined on compounds but I do not want to affect the default persisting behaviour on the compound children in such a case.

Obviously this causes confusions. I don't want to immediately change the behaviour but instead let your input settle a little bit. Maybe @jensens has some more input to this subject. I am not totally against changing the behaviour, but if we do we should do it right once

If we decide to keep the current behaviour we should think about a proper argument naming and provide a clear documentation which is missing in this case without question.