{"action":"created","issue":{"url":"https://api.github.com/repos/bluedynamics/yafowil/issues/29","repository_url":"https://api.github.com/repos/bluedynamics/yafowil","labels_url":"https://api.github.com/repos/bluedynamics/yafowil/issues/29/labels{/name}","comments_url":"https://api.github.com/repos/bluedynamics/yafowil/issues/29/comments","events_url":"https://api.github.com/repos/bluedynamics/yafowil/issues/29/events","html_url":"https://github.com/bluedynamics/yafowil/issues/29","id":195544271,"number":29,"title":"How should the `writer=` override work?","user":{"login":"jean","id":84800,"avatar_url":"https://avatars.githubusercontent.com/u/84800?v=3","gravatar_id":"","url":"https://api.github.com/users/jean","html_url":"https://github.com/jean","followers_url":"https://api.github.com/users/jean/followers","following_url":"https://api.github.com/users/jean/following{/other_user}","gists_url":"https://api.github.com/users/jean/gists{/gist_id}","starred_url":"https://api.github.com/users/jean/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jean/subscriptions","organizations_url":"https://api.github.com/users/jean/orgs","repos_url":"https://api.github.com/users/jean/repos","events_url":"https://api.github.com/users/jean/events{/privacy}","received_events_url":"https://api.github.com/users/jean/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":9,"created_at":"2016-12-14T14:26:26Z","updated_at":"2016-12-15T09:25:02Z","closed_at":null,"body":"I wanted to add the following test:\r\n\r\n```\r\nIf ``persist_writer`` is provided by the widget, passing ``writer`` overrides it::\r\n\r\n    >>> form['yet_another_field'] = factory(\r\n    ...     'text',\r\n    ...     props={\r\n    ...         'persist': True,\r\n    ...         'persist_writer': write_mapping_writer\r\n    ...     })\r\n    >>> data = form.extract(request={\r\n    ...     'form.yet_another_field': 'value',\r\n    ... })\r\n    >>> model = MixedModel()\r\n    >>> data['yet_another_field'].write(model, writer=attribute_writer)\r\n    >>> model.yet_another_field\r\n    'value'\r\n```\r\n\r\nBut this fails:\r\n\r\n```\r\nFile \"/home/jean/repos/git/yafowil/src/yafowil/persistence.rst\", line 182, in persistence.rst\r\nFailed example:\r\n    model.yet_another_field\r\nException raised:\r\n    Traceback (most recent call last):\r\n      File \"/usr/lib/python2.7/doctest.py\", line 1315, in __run\r\n        compileflags, 1) in test.globs\r\n      File \"<doctest persistence.rst[63]>\", line 1, in <module>\r\n        model.yet_another_field\r\n    AttributeError: 'MixedModel' object has no attribute 'yet_another_field'\r\n```\r\n\r\nIf I understand correctly, it works like this:\r\n\r\n- if the form has `self.persist_writer`, that is used throughout.\r\n- if `writer=` is passed, that is used for child nodes that *don't* have `self.persist_writer` set. \r\n- but `writer=` is not used for nodes that *already have* `self.persist_writer` set.\r\n\r\nThat 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?"},"comment":{"url":"https://api.github.com/repos/bluedynamics/yafowil/issues/comments/267278537","html_url":"https://github.com/bluedynamics/yafowil/issues/29#issuecomment-267278537","issue_url":"https://api.github.com/repos/bluedynamics/yafowil/issues/29","id":267278537,"user":{"login":"rnixx","id":246021,"avatar_url":"https://avatars.githubusercontent.com/u/246021?v=3","gravatar_id":"","url":"https://api.github.com/users/rnixx","html_url":"https://github.com/rnixx","followers_url":"https://api.github.com/users/rnixx/followers","following_url":"https://api.github.com/users/rnixx/following{/other_user}","gists_url":"https://api.github.com/users/rnixx/gists{/gist_id}","starred_url":"https://api.github.com/users/rnixx/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rnixx/subscriptions","organizations_url":"https://api.github.com/users/rnixx/orgs","repos_url":"https://api.github.com/users/rnixx/repos","events_url":"https://api.github.com/users/rnixx/events{/privacy}","received_events_url":"https://api.github.com/users/rnixx/received_events","type":"User","site_admin":false},"created_at":"2016-12-15T09:25:02Z","updated_at":"2016-12-15T09:25:02Z","body":"@jean Your objections are valid and I'm happy that you point it out. Things can only get better if they are discussed ;)\r\n\r\nActually ``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.\r\n\r\nIn 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__``.\r\n\r\nA 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.\r\n\r\nObviously 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_\r\n\r\nIf 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."}}
{"actor":{"display_login":"rnixx"}}