Arguments to display() called as instance method are ignored

[Cito]

I found another small issue with the display() method of the Widget class. Basically, the current code looks like this:

class Widget(pm.Parametered):

    @util.class_or_instance
    def display(self, cls, value=None, displays_on=None, **kw):

        if value is not None and 'value' not in kw:
            kw['value'] = value

        if not self:
            self = cls.req(**kw)

The problem here is that the cls and kw arguments are completely ignored when display() is called on a Widget instance. Is this really intended? I had expected you could pass arguments to an instance as well. But for that to work, the code must be changed like this:

class Widget(pm.Parametered):

    @util.class_or_instance
    def display(self, cls, value=None, displays_on=None, **kw):

        if value is not None and 'value' not in kw:
            kw['value'] = value

        if self:
            for key, value in kw.items():
                setattr(self, key, value)
        else:
            self = cls.req(**kw)

Or, if the value und kw arguments really shall be used only when called as a class method, then this should be documented and the code should be changed to this:

class Widget(pm.Parametered):

    @util.class_or_instance
    def display(self, cls, value=None, displays_on=None, **kw):

        if not self:
            if value is not None and 'value' not in kw:
                kw['value'] = value
            self = cls.req(**kw)

[ralphbean]

I would also expect you to be able to pass arguments to an instance which leads me to the first of the two options.

The widget instances are mostly meant to be internal (even though .req() appears in the docs). Even so, methods decorated with class_or_instance should behave just that way; they should behave indifferently whether or not they are called on a class or instance.

@paj28, would you like to weigh in?

[Cito]

The point that widgets are meant to be used as classes should be better explained in the docs, I did not get it at first reading. There is a passage in the docs that says:

"ToscaWidgets 2 creates a new instance of a widget every time it is used in a request. This allows widget and application code to update widget attributes in the natural, pythonic manner, ..."

In the beginning, when trying out TW2, I had been mislead by the words "and application code" to believe that widgets are intended to be used as instances by the application. Because otherwise, if the application works only with classes, and instances are created only internally, then application code cannot benefit from the pythonic way of updating widget attributes, only widget code (overwritten methods) can.

Also, if the application is supposed to work with classes only, we need something like the child_args argument of TW1 for making it easier to do standard things like setting options of SelectionFields dynamically. We discussed that earlier, maybe I can work something out this week.

[amol-]

child_args was really messy, I feel that the new prepare() provides a cleaner way to prepare widgets for display and it is probably better to avoid having too many ways to do the same thing. It would create chaos (as we are already seeing with the req/class issue) and make harder to maintain documentation and codebase.

I agree that documentation is a bit lacking on this and we should add a few examples for example to update single select options at display time and so on. Currently users don't have a set of best practices and will end inventing ways which the tw2 team might have not thought of resulting in disoriented users

[paj28]

I think the first proposed change is acceptable.

We are hitting a deeper issue around whether req() should be private - but that's a bigger issue to solve, and I don't think the changed proposed here makes the situation any worse.

More examples in the docs always welcome. Although the tw2 docs are better than tw1, one area they've fallen down on is the lack of an "advanced tutorial".