Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properties: add adapter for callables #522

Closed
wants to merge 1 commit into from

Conversation

tardyp
Copy link
Member

@tardyp tardyp commented Sep 11, 2012

it is a very common need for users just to want to create a
function to do the property interpolation.

one can now say:

Shell(command=lambda p:"echo next build is"+(p.getProperty("buildnumber")+1))

it is a very common need for users just to want to create a
function to do the property interpolation.

one can now say:

Shell(command=lambda p:"echo next build is"+(p.getProperty("buildnumber")+1))

Signed-off-by: Pierre Tardy <pierre.tardy@intel.com>
@djmitche
Copy link
Member

I like this! Can you add some docs once the Interpolate docs are in?

@tomprince
Copy link
Member

I feel that this should be a decorator, rather than an adapter. I don't have a good reason for this, but that is why I didn't implement this originally.

I think part of the reason is that it isn't obvious to me that the signature that this expects is the only/most common reasonable one, and so I didn't want to commit to a particular interpretation.

@tomprince
Copy link
Member

Also, I think that whatever this ends up looking like, it should land in 0.8.7.

@tomprince
Copy link
Member

Another benefit to not having something like this, is that it exposes people to the simplicity of writing renderables.

@tardyp
Copy link
Member Author

tardyp commented Sep 12, 2012

while renderable is simple and elegant, it still uses advanced python semantics.

I think we should not require our users to use decorators, or even to override classes when it is not stricly needed.

I like the syntax sugar of adapters, and the fact that it just works.

@djmitche
Copy link
Member

This is also one-line syntax, where decorators and/or renderables require more verbiage.

@tomprince
Copy link
Member

Something like

@simpleRenderer
def NextBuild(props):
    return props.getProperty("buildnumber") + 1
Shell(command=["echo", Interpolate("next build is %s", NextBuild())])

seems like better code, even if it is slightly more verbose.

@tardyp
Copy link
Member Author

tardyp commented Sep 13, 2012

The lambda fans could also write

Shell(command=["echo", Interpolate("next build is %s", 
                          simpleRenderer(lambda p:p.getProperty("buildnumber") + 1)])

I think both adaptor VS decorator approaches make sense. dustin to decide..

@jaredgrubb
Copy link
Member

I personally really like the decorator. As tardyp mentions, the lambda version works with decorator, and the decorator feels more pythonic as a solution.

And for naming, can we call the decorator @withProperties. (ducks. im kidding.)

@tomprince
Copy link
Member

def withProperties(f):
    @implementer(IRenderable)
    @provider(IRenderable)
    class _withProperties(object):
        @staticmethod
        def getRenderingFor(props):
            return f(props)
    return _withProperties

I'm disappointed that @jaredgrubb was kidding, since that name is probably the ideal name (except of course for the fact that we have an existing class WithProperties.

@jaredgrubb
Copy link
Member

@tomprince .. it was the first thing that came to my mind! You've been working so hard to dismantle and deprecate WithProperties that proposing to backdoor it back in seemed like a joke thing to do... I do kinda like it -- with the worry that it might be confusing ("no, you have to use the one with the small 'w'; the big 'W' doesnt work anymore...").

@djmitche
Copy link
Member

I think we're all agreed that explicit is better than implicit here: a decorator that can also be used inline as in @tardyp's example above.

I think we'll end up getting old and gray telling people about withProperties and WithProperties. And I don't find it particularly well-suited to the purpose. How about just renderer?

@properties.renderer
def def nextBuild(props):
    return props.getProperty("buildnumber") + 1
Shell(command=["echo", Interpolate("next build is %s", nextBuild())])

or

Shell(command=["echo", Interpolate("next build is %s",
             properties.renderer(lambda props : props.getProperty('buildnumber')+1))])

@tomprince
Copy link
Member

I certainly agree that withProperties would cause too much confusion. I think renderer is a not unreasonable name.

@tardyp
Copy link
Member Author

tardyp commented Sep 21, 2012

abandon this change in favor of properties.renderer

@tardyp tardyp closed this Sep 21, 2012
@djmitche
Copy link
Member

I just merged @renderer to buildbot-0.8.7.

mzdaniel pushed a commit to mzdaniel/buildbot that referenced this pull request Aug 2, 2013
chapuni pushed a commit to chapuni/buildbot that referenced this pull request Nov 4, 2013
This decorator was suggested by tomprince:
  buildbot#522 (comment)
and has been modified, tested, and documented here.

This commit also reorganizes some of the properties documentation,
splitting that in "Customization" between the "Properties" section and
the "Classes" section.
@tardyp tardyp deleted the callable branch December 21, 2013 17:52
thinkski pushed a commit to kuna-systems/buildbot that referenced this pull request Sep 10, 2014
This decorator was suggested by tomprince:
  buildbot#522 (comment)
and has been modified, tested, and documented here.

This commit also reorganizes some of the properties documentation,
splitting that in "Customization" between the "Properties" section and
the "Classes" section.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants