Code review comment for lp://qastaging/~ted/snapcraft/ros-catkin-plugin

On Thu, 2015-10-15 at 23:44 +0000, Sergio Schvezov wrote:

> One thing we haven't been doing and I started addressing was that all our methods our public and I'd make them private (prefix the method with _), this becomes more important now that we inherit from plugins instead of 'requiring' them.

Fixed.

> > +binaries:
> > + listener:
> > + exec: opt/ros/indigo/beginner_tutorials/listener
> > + talker:
> > + exec: opt/ros/indigo/beginner_tutorials/talker
> > +
> > +parts:
> > + roscore:
> > + plugin: roscore
> > + catkin-tutorials:
> > + plugin: catkin
> > + source: .
> > + catkin-packages:
> > + - beginner_tutorials
> > +
> > +services:
>
> just a minor thing, but maybe put this upstairs (above) together with binaries

Fixed

> > + rosmaster:
> > + start: bin/roscore-rosmaster-service
> > + description: ROS Master Service
> > + ports:
>
> this can be dropped, I think it is being killed and nothing uses it so it can just go away to not confuse people (ports and everything under it)

Fixed, we should probably mark it deprecated in the schema then, no?

> > + def __init__(self, name, options):
> > + super().__init__(name, options)
> > + self.rosversion = options.rosversion
>
> the base class already saves options as self.options from way back and I guess we never noticed that, but maybe all these 'options' attribs don't need specific assigns.

Fixed.

> > + except lxml.etree.ParseError:
> > + logger.warning("Unable to read packages.xml file for '" + pkg + "'")
>
> use .format instead of + or maybe ('string %r' % pkg)

Fixed.

> > + return
>
> what are the consequences of this, even if handled internally maybe raising a RuntimeError is good here and logging from whoever calls this.

My thoughts here is that we can let the developer decide, if they care
that we can't parse it they can fix the file. If they don't care and
they just want to add the dependencies via stage-packages manually we
should allow that too. I kinda see packages.xml as nicety instead of a
requirement for a build.

> > +
> > + for deptype in ['buildtool_depend', 'build_depend', 'run_depend']:
>
> use () instead of [] here.

Fixed. But I don't understand why that would be better, it seems like a
list would always be less memory?