<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>
<p dir="ltr">Thanks, <br>
Vasily</p>
<div class="gmail_quote">17 февр. 2016 г. 3:52 пользователь "Dustin J. Mitchell" <<a 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 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 href="mailto:devel@buildbot.net">devel@buildbot.net</a><br>
> <a 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 href="mailto:devel@buildbot.net">devel@buildbot.net</a><br>
<a href="https://lists.buildbot.net/mailman/listinfo/devel" rel="noreferrer" target="_blank">https://lists.buildbot.net/mailman/listinfo/devel</a><br>
<br>
</blockquote></div>