[Webkit-unassigned] [Bug 37418] Add experimental prototype Rietveld integration to webkit-patch upload

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=37418





--- Comment #7 from Adam Barth <abarth at webkit.org>  2010-04-11 17:09:41 PST ---
All good comments, thanks.

> Thanks for getting the ball rolling on this, Adam!  It's amazing you were able
> to do this with so little code.

We have lots of infrastructure now that makes adding features pretty easy.

> +++ b/WebKitTools/Scripts/webkitpy/common/config/__init__.py

> Until we figure out how we want to use our __init__ files, I would recommend
> leaving the file empty and putting this information in a separate file.

Ok.  Maybe servers.py in the config folder?  I tried that with the irc
settings, but its slightly awkward to import:

import webkitpy.common.config.servers as config_servers

???

>  Also,
> I've mentioned this before, but I have mixed feelings about putting settings
> for a range of areas in a single config folder because it increases the number
> of couplings.  I think it might be better, for example, to put Rietveld-related
> settings in a rietveld/ folder.

For Bugzilla, we store this information directly in bugzilla.py, but there's a
comment there that Eric added about wanting to move this to a more generic
location.  I think Eric has the idea that someone might want to take webkitpy
and use it for their own project.  They might want a central place to point all
the URLs, etc, to the ones for their project.  I'm not sure what's best here.

> Should we still be using deprecated_logging in new code?

No, but I don't know how to use the real logging.  I need to read up more on
it.

> Is the Apache license compatible with the WebKit project?

My understanding is that it is because it's essentially the same as the BSD
license.

> Given that WebKitPatch is meant to be a generic all-purpose tool, I wonder if
> there's a way to design things so the class doesn't itself have to be aware of
> all its capabilities a priori (e.g. if there is some sort of generic
> pass-through design where you can add more functionality to WebKitPatch without
> having to change its class structure/attributes each time).

That would be nice.  We do something like that with commands and it works
nicely.  We just haven't gotten around to build that for the main tool object.

> Also, what's the rationale for using MockExecute() here but Mock() in
> queues_unittest.py?

We could use MockExecutive in queues_unittest.  I could have just used None in
that case (and maybe should have) because the method I'm testing doesn't
interact with the executive.

I eventually want to build a nice fancy MockExecutive and have more of the
tool-related classes use that as their mock point instead of always mocking at
the tool.  Rietveld is a bit of an experiment in that regard.  I just did the
minimum necessary to make that work in this patch.

> +        codereview_issue = state.get("codereview_issue")
> 
> Where is this string coming from -- is there a way to avoid having it
> hard-coded in more than one spot?

The state dictionary is a bit of an experiment.  We probably should make it
into an object and document it properly.  As the moment, this is a hook for
future extension.  I wanted to do try remembering the issue number, but I
wasn't sure of the bets design and so pushed it off to the next iteration.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.