[devel at bb.net] Review request for bug2340 branch ("slave"->"worker" renaming in master)

[Vasily]

As for one-liners - maybe something like
   attr_name = [name for (name, value) in scope.iteritems() if value ==
attribute][0]
? Does not rely on order stability and uses less memory :-)

Thanks,
Vasily
17 февр. 2016 г. 3:52 пользователь "Dustin J. Mitchell" <dustin at v.igoro.us>
написал:

> I had a look at this.  I've also been following along pretty closely,
> so a lot of this isn't new to me.  This is some great code, and a
> really solid and well-organized approach to the problem.
>
> I would love to get sa2ajj's input in particular about plugins, and
> another set of eyes from mine would be helpful for such a large-scale
> change as this.  So please don't take my review as indicating this
> doesn't need your attention!
>
> Dustin
>
> ===
>
> DB migration looks great
>
> ===
>
> Old names fallback support
>
> Nicely done; minor comments:
>
>
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L56
> If `name` is not specified old name is construct by replacing:
> "worker" -> "slave",
> "Worker" -> "Slave".
>
> I think you mean `compat_name` here?
>
> ---
>
>
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py#L199
> attribute_name = scope.keys()[scope.values().index(attribute)]
>
> Does Python guarantee stable ordering of values() and keys()?  If not,
> I suspect there are better one-liners for this inverted lookup.
>
> ===
>
> Warnings catching helpers
>
> Also nicely done
>
> ===
>
> Fallbacks support in Plugins
>
> Looks good - I'd like to hear from sa2ajj, who may also be able to
> answer the TODO question in the tests
>
> ===
>
> Fallback for properties
>
> This is really elegant.  Yes, Properties, Interpolate, and
> WithProperties are the three cases I know of where properties are
> read.
>
> ===
>
> Dustin
>
>
> On Mon, Feb 15, 2016 at 5:25 AM, Vladimir Rutsky
> <rutsky.vladimir at gmail.com> wrote:
> > Hello!
> >
> > I finished main parts of renaming in master and asking to review my
> changes.
> > Few more little changes are expected to land in that branch, but they can
> > be applied and after bug2340 branch merging.
> >
> > Main goal of this branch is to rename all "slave" to "worker" in master
> part
> > of
> > Buildbot (i.e. buildslave module and communication with it from master
> are
> > not touched). API that is available in eight branch is changed in
> backward
> > compatible way, API that were introduced in nine branch renamed without
> > providing fallback.
> > Buildbot should be completely function in bug2340 branch, changes in
> master
> > branch are merged into it.
> >
> > Actual list of topics to take a look at available in PR description:
> > https://github.com/buildbot/buildbot/pull/1962
> >
> > Review high-level API changes
> > -----
> >
> > - [x] PR with high-level changes for review:
> > https://github.com/buildbot/buildbot/pull/1961
> > (done by Dustin)
> >
> > Review Database schema migration
> > -----
> >
> > - [ ]
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/migrate/versions/045_worker_transition.py
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_db_migrate_versions_045_worker_transition.py
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/db/model.py
> >
> >      This also fixes http://trac.buildbot.net/ticket/3088.
> >
> > Review old names fallback support
> > -----
> > - [ ] Methods for providing fallbacks implemented here:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/worker_transition.py
> >
> >       Tests for them:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_worker_transition.py
> >
> > Review warnings catching helpers
> > -----
> >
> > - [ ] Warnings catching helpers:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/util/warnings.py
> >
> >       Tests:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_test_util_warnings.py
> >
> > Review fallbacks support in Plugins
> > -----
> >
> > - [ ]
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/db.py#L365
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/plugins/__init__.py#L42
> >
> >       Tests:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_plugins.py#L284
> >
> > Review fallback for properties
> > -----
> >
> > - [ ] Take a look at
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/process/properties.py#L429
> > and check usage of `_on_property_usage()` in this module.
> >
> >       Tests:
> >
> https://github.com/buildbot/buildbot/blob/bug2340/master/buildbot/test/unit/test_process_properties.py#L1521
> >
> > Review raw diff
> > -----
> >
> > - [ ] To get raw diff via git:
> >
> > git clone https://github.com/buildbot/buildbot.git
> > cd buildbot
> > git fetch --all
> > git diff origin/master...origin/bug2340
> >
> >
> > Regards,
> >
> > Vladimir Rutsky
> >
> > _______________________________________________
> > devel mailing list
> > devel at buildbot.net
> > https://lists.buildbot.net/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel at buildbot.net
> https://lists.buildbot.net/mailman/listinfo/devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.buildbot.net/pipermail/devel/attachments/20160217/08ead7d0/attachment.html>