Skip to main content

[pkg-discuss] Re: review request: li test coverage improvenents

  • From: Edward Pilatowicz < >
  • To: Erik Trauschke < >
  • Cc: " " < >
  • Subject: [pkg-discuss] Re: review request: li test coverage improvenents
  • Date: Mon, 24 Feb 2014 15:54:52 -0800

comments inline below.
thanks
ed

On Mon, Feb 24, 2014 at 03:39:19PM -0800, Erik Trauschke wrote:
> Hi Ed,
>
> On 02/24/14 10:45 AM, Edward Pilatowicz wrote:
> >hey erik,
> >
> >i attempted to addressed all your comments below.
> >most of the changes were pretty strait forward.  here's a new webrev:
> >
> >     http://mcescher.us.oracle.com/export/ws/pkg.lialtroot/webrev
> >
> >and here's a delta webrev:
> >
> >     http://mcescher.us.oracle.com/export/ws/pkg.lialtroot/webrev.diff
>
> src/modules/client/linkedimage/common.py:
>
> 476, 1423
>       Again, I don't mind using comments which are just clauses instead
> not proper sentences but please don't make hybrids between the two
> ;)
>

476 has:

                # Check if this is our first time accessing the current image
                # or if we're just re-initializing ourselves.

which seems ok to me.

1423 has:

                # Set path related properties.  we use self.__root in place of
                # current_path() since we may not actually be linked yet.

where i forgot to capatilize the "we" on the first line.  i've fixed this.

> Otherwise this looks good to me.
>
> >fyi, i haven't re-synced with the gate because i didn't want to have
> >spurious delta.  if you're ok with these changes i'll re-sync and
> >putback.
> >
> >thanks,
> >ed
> >
> >ps - i just noticed that you replied off-alias, so i'm keeping this
> >reply off-alias and the new webrev links below are internal.  of course
> >we're supposed to be doing things in public, so i'd appreciate it if you
> >could publicly reply to my email on alias saying we did an offline
> >review and you're happy with my changes.  that way when i putback
> >there's a public record of someone having reviewed my changes.  ;)
>
> I think I replied to your ping which was just sent to me, sorry about this.
>
> Erik
>
> >
> >
> >On Mon, Feb 10, 2014 at 04:31:56PM -0800, Erik Trauschke wrote:
> >>
> >>On 02/10/14 11:24 AM, Edward Pilatowicz wrote:
> >>>ha.  i didn't realize you were on vacation.  have fun.  ;)
> >>>ed
> >>>
> >>>On Mon, Feb 10, 2014 at 07:38:36AM -0800, Erik Trauschke wrote:
> >>>>Sorry about that.
> >>>>I'll go scuba diving for a bit now but will finish this this afternoon.
> >>>>
> >>>>Erik
> >>>>
> >>>>On 02/ 7/14 04:54 PM, Edward Pilatowicz wrote:
> >>>>>ping.
> >>>>>ed
> >>>>>
> >>>>>On Thu, Jan 23, 2014 at 11:40:58AM -0800, Erik Trauschke wrote:
> >>>>>>Hi Ed,
> >>>>>>
> >>>>>>I'll have a look at this.
> >>>>>>
> >>>>>>Erik
> >>>>>>
> >>>>>>On 01/16/14 04:52 PM, Edward Pilatowicz wrote:
> >>>>>>>ping...
> >>>>>>>ed
> >>>>>>>
> >>>>>>>On Mon, Jan 06, 2014 at 01:17:45AM -0800, Edward Pilatowicz wrote:
> >>>>>>>>hey all,
> >>>>>>>>
> >>>>>>>>while working on my last wad (inherited facet support for linked 
> >>>>>>>>images)
> >>>>>>>>i was looking at test suite code coverage for the linked images
> >>>>>>>>subsystem and i noticed the following problems:
> >>>>>>>>
> >>>>>>>>   18025813 remove dead linked images code
> >>>>>>>>   18025839 linked image altroot code is untested
> >>>>>>>>
> >>>>>>>>the following changes address these issues:
> >>>>>>>>
> >>>>>>>>   https://ips.java.net/webrev/edp/18025839.0/
> >>>>>>>>
> >>>>>>>>these changes improve test coverage as following:
> >>>>>>>>
> >>>>>>>>   linkedimage/zone:   70% -> 90%
> >>>>>>>>   linkedimage/common: 80% -> 87%
> >>
> >>generally:
> >>
> >>1.) I think you do that all the time in comments and in emails, but
> >>if you actually use punctuation for sentences in comments I'd rather
> >>see capital letters at the beginning. Or don't use periods at the
> >>end and not make it a real sentence in the first place ;)
> >>
> >
> >yeah.  sorry about that.  normally i try to self-review for that before
> >sending a webrev out and in this case i forgot.  i edited my patch to
> >fix this.
> >
> >>2.) It seems you spend a lot time and code making sure that your
> >>path ends with a /. It does look like this is done the same way
> >>paths are stored in an Image object (not sure though) but is this
> >>really necessary? Wouldn't it be easier to bother with the trailing
> >>/ at the places where this gets actually processed?
> >>I see in a few cases (common.py:485++) you just pass the variables
> >>to os.path.join() afterwards which should do all the separator
> >>handling for you.
> >>
> >
> >i'd prefer to keep this.  i do this for a couple reasons.  it allow for
> >easy differentiation between directory and file paths in the code and in
> >configuration files.  if you know you're expcting a directory you can
> >easily assert that.  also, it means that if you ever accidently try to
> >create a file using a directory path, the operation will fail.  i don't
> >think that the performance of maniuplating these paths is causing any
> >problems, so i'd prefer to stick with this convention.
> >
> >>
> >>src/modules/client/api_errors.py
> >>2844, 2851
> >>    I think you don't have to cast into str explicitly here
> >>
> >
> >fixed.
> >
> >>src/modules/client/linkedimage/common.py
> >>611++
> >>    Isn't this checking the same thing twice? First you assert that
> >>either path' or 'current_path' is set, then you check this again at
> >>line 616 and 621.
> >>
> >
> >yeah.  sorry.  sometimes i get can get carried away with assertions.
> >i've remove the ones on line 616 and 621.
> >
> >>678
> >>    # we have *a* transform from our plugins
> >>
> >
> >fixed
> >
> >>src/modules/client/linkedimage/system.py
> >>138
> >>    Maybe remove the the whole function altogether?
> >>
> >
> >can't.  the LinkedImagePlugin and LinkedImageChildPlugin classes, which
> >are the parent classes for pluings, are basically templates and hence all
> >their methods raise NotImplementedError if invoked.  this forces child
> >classes to implement those methods.
> >
> >>src/tests/cli/t_pkg_linked.py
> >>3264
> >>    Is this docstring still valid?
> >>
> >
> >Nope.
> >
> >>3538
> >>    You really need another wrapper for this just to assert if the
> >>input is a str? Maybe just add this to cmdline_run?
> >>
> >
> >this function doesn't exist to have the assert.  it exists to set
> >coverage to false.  without this function there would be lots more calls
> >to cmdline_run() that would repeatedly have to set coverage=False.
> >
> >
> >>3544, 3559
> >>    needs docstring
> >>
> >
> >done.
> >
> >>3650+
> >>    I think the docstring should say: "You can't link images to 
> >> themselves."
> >>
> >
> >done.
> >


[pkg-discuss] Re: review request: li test coverage improvenents

Erik Trauschke 02/24/2014

[pkg-discuss] Re: review request: li test coverage improvenents

Edward Pilatowicz 02/24/2014
 
 
Close
loading
Please Confirm
Close