<html>
<head>
<meta content="text/html; charset=utf-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">On 17.02.2016 11:14, Vasily wrote:<br>
</div>
<blockquote
cite="mid:CABiDpkk1ex-qzqf_8Y7RpYWHer0pE_nvRVj-rUwmBR53CJWPVA@mail.gmail.com"
type="cite">
<p dir="ltr">As for one-liners - maybe something like <br>
attr_name = [name for (name, value) in scope.iteritems() if
value == attribute][0]<br>
? Does not rely on order stability and uses less memory :-)</p>
</blockquote>
<br>
Your one-lined is longer :)<br>
<br>
Since order stability is guaranteed in Python 2/3<br>
(<a class="moz-txt-link-freetext" href="https://docs.python.org/2.7/library/stdtypes.html?highlight=values#dict.items">https://docs.python.org/2.7/library/stdtypes.html?highlight=values#dict.items</a>),<br>
I don't see a reason to change it.<br>
Memory consumption or performance shouldn't be an issue in this
part, <br>
that runs during module loading.<br>
<br>
Thank for your comment,<br>
<br>
Vladimir<br>
<br>
<blockquote
cite="mid:CABiDpkk1ex-qzqf_8Y7RpYWHer0pE_nvRVj-rUwmBR53CJWPVA@mail.gmail.com"
type="cite">
<p dir="ltr">Thanks, <br>
Vasily</p>
<div class="gmail_quote">17 февр. 2016 г. 3:52 пользователь
"Dustin J. Mitchell" <<a moz-do-not-send="true"
href="mailto:dustin@v.igoro.us">dustin@v.igoro.us</a>>
написал:<br type="attribution">
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">I had a look
at this. I've also been following along pretty closely,<br>
so a lot of this isn't new to me. This is some great code,
and a<br>
really solid and well-organized approach to the problem.<br>
<br>
I would love to get sa2ajj's input in particular about
plugins, and<br>
another set of eyes from mine would be helpful for such a
large-scale<br>
change as this. So please don't take my review as indicating
this<br>
doesn't need your attention!<br>
<br>
Dustin<br>
<br>
===<br>
<br>
DB migration looks great<br>
<br>
===<br>
<br>
Old names fallback support<br>
<br>
Nicely done; minor comments:<br>
<br>
<a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56</a><br>
If `name` is not specified old name is construct by replacing:<br>
"worker" -> "slave",<br>
"Worker" -> "Slave".<br>
<br>
I think you mean `compat_name` here?<br>
<br>
---<br>
<br>
<a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199</a><br>
attribute_name = scope.keys()[scope.values().index(attribute)]<br>
<br>
Does Python guarantee stable ordering of values() and keys()?
If not,<br>
I suspect there are better one-liners for this inverted
lookup.<br>
<br>
===<br>
<br>
Warnings catching helpers<br>
<br>
Also nicely done<br>
<br>
===<br>
<br>
Fallbacks support in Plugins<br>
<br>
Looks good - I'd like to hear from sa2ajj, who may also be
able to<br>
answer the TODO question in the tests<br>
<br>
===<br>
<br>
Fallback for properties<br>
<br>
This is really elegant. Yes, Properties, Interpolate, and<br>
WithProperties are the three cases I know of where properties
are<br>
read.<br>
<br>
===<br>
<br>
Dustin<br>
<br>
<br>
On Mon, Feb 15, 2016 at 5:25 AM, Vladimir Rutsky<br>
<<a moz-do-not-send="true"
href="mailto:rutsky.vladimir@gmail.com">rutsky.vladimir@gmail.com</a>>
wrote:<br>
> Hello!<br>
><br>
> I finished main parts of renaming in master and asking to
review my changes.<br>
> Few more little changes are expected to land in that
branch, but they can<br>
> be applied and after bug2340 branch merging.<br>
><br>
> Main goal of this branch is to rename all "slave" to
"worker" in master part<br>
> of<br>
> Buildbot (i.e. buildslave module and communication with
it from master are<br>
> not touched). API that is available in eight branch is
changed in backward<br>
> compatible way, API that were introduced in nine branch
renamed without<br>
> providing fallback.<br>
> Buildbot should be completely function in bug2340 branch,
changes in master<br>
> branch are merged into it.<br>
><br>
> Actual list of topics to take a look at available in PR
description:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/pull/1962"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/pull/1962</a><br>
><br>
> Review high-level API changes<br>
> -----<br>
><br>
> - [x] PR with high-level changes for review:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/pull/1961"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/pull/1961</a><br>
> (done by Dustin)<br>
><br>
> Review Database schema migration<br>
> -----<br>
><br>
> - [ ]<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/migrate/versions/045_worker_transition.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/migrate/versions/045_worker_transition.py</a><br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_db_migrate_versions_045_worker_transition.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_db_migrate_versions_045_worker_transition.py</a><br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py</a><br>
><br>
> This also fixes <a moz-do-not-send="true"
href="http://trac.buildbot.net/ticket/3088" rel="noreferrer"
target="_blank">http://trac.buildbot.net/ticket/3088</a>.<br>
><br>
> Review old names fallback support<br>
> -----<br>
> - [ ] Methods for providing fallbacks implemented here:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py</a><br>
><br>
> Tests for them:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py</a><br>
><br>
> Review warnings catching helpers<br>
> -----<br>
><br>
> - [ ] Warnings catching helpers:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py</a><br>
><br>
> Tests:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py</a><br>
><br>
> Review fallbacks support in Plugins<br>
> -----<br>
><br>
> - [ ]<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365</a><br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42</a><br>
><br>
> Tests:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284</a><br>
><br>
> Review fallback for properties<br>
> -----<br>
><br>
> - [ ] Take a look at<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429</a><br>
> and check usage of `_on_property_usage()` in this module.<br>
><br>
> Tests:<br>
> <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521</a><br>
><br>
> Review raw diff<br>
> -----<br>
><br>
> - [ ] To get raw diff via git:<br>
><br>
> git clone <a moz-do-not-send="true"
href="https://github.com/buildbot/buildbot.git"
rel="noreferrer" target="_blank">https://github.com/buildbot/buildbot.git</a><br>
> cd buildbot<br>
> git fetch --all<br>
> git diff origin/master...origin/bug2340<br>
><br>
><br>
> Regards,<br>
><br>
> Vladimir Rutsky<br>
><br>
> _______________________________________________<br>
> devel mailing list<br>
> <a moz-do-not-send="true"
href="mailto:devel@buildbot.net">devel@buildbot.net</a><br>
> <a moz-do-not-send="true"
href="https://lists.buildbot.net/mailman/listinfo/devel"
rel="noreferrer" target="_blank">https://lists.buildbot.net/mailman/listinfo/devel</a><br>
_______________________________________________<br>
devel mailing list<br>
<a moz-do-not-send="true" href="mailto:devel@buildbot.net">devel@buildbot.net</a><br>
<a moz-do-not-send="true"
href="https://lists.buildbot.net/mailman/listinfo/devel"
rel="noreferrer" target="_blank">https://lists.buildbot.net/mailman/listinfo/devel</a><br>
<br>
</blockquote>
</div>
</blockquote>
<br>
</body>
</html>