[devel at bb.net] Review request for bug2340 branch ("slave"->"worker" renaming in master)
Vladimir Rutsky
rutsky.vladimir at gmail.com
Sat Feb 20 00:25:11 UTC 2016
On 17.02.2016 11:14, Vasily wrote:
>
> 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 :-)
>
Your one-lined is longer :)
Since order stability is guaranteed in Python 2/3
(https://docs.python.org/2.7/library/stdtypes.html?highlight=values#dict.items),
I don't see a reason to change it.
Memory consumption or performance shouldn't be an issue in this part,
that runs during module loading.
Thank for your comment,
Vladimir
> Thanks,
> Vasily
>
> 17 февр. 2016 г. 3:52 пользователь "Dustin J. Mitchell"
> <dustin at v.igoro.us <mailto: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 <mailto: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 <mailto:devel at buildbot.net>
> > https://lists.buildbot.net/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel at buildbot.net <mailto: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/20160220/fde21664/attachment.html>
More information about the devel
mailing list