Skip to main content

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

  • From: Shawn Walker < >
  • To:
  • Cc: Tim Foster < >, Yiteng Zhang < >
  • Subject: [pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed
  • Date: Fri, 23 Aug 2013 12:52:18 -0700

On 08/22/13 21:07, Tim Foster wrote:
Hi Yiteng,

On 08/23/13 11:18 AM, Yiteng Zhang wrote:
http://ips.java.net/webrev/yitezhan/16851082_4/

I've had a look at this. Here's some feedback:
...
line 539 - this might be easier to read as:

  release, branch, vers, ts = \
     version.Version.split(vers)[0]
  pfmri = "pkg://%s/%s@%s-%s:%s" % (pub, stem, release, vers, ts)

This is a little bit off, 'vers' above is actually the branch, so this is what you really need:

   release, build_release, branch, ts = \
      version.Version.split(vers)[0]
   pfmri = "pkg://%s/%s@%s-%s:%s" % (pub, stem, release, branch, ts)

src/pull.py
line 898 - given the use of spaces for indents in the message, I wonder
is this something we should be localizing, so something like:

  msg(_("   %s") % f.get_fmri(anarchy=True,
      include_build=False))

instead.  I'm not sure of this, but it's something I'd question (Takeshi?)

In this particular case, we should drop the _() as there's nothing to localise here. And Tim's right, if we were using _() we shouldn't include the leading whitespace in a localised string.

I'll look at the fifth webrev you've posted and respond separately. [1]

-Shawn

http://ips.java.net/webrev/yitezhan/16851082_5


[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

(continued)

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Erik Trauschke 08/23/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Yiteng Zhang 08/26/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Erik Trauschke 08/27/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Shawn Walker 08/27/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Yiteng Zhang 08/27/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Erik Trauschke 08/27/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Yiteng Zhang 08/28/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Erik Trauschke 08/30/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Yiteng Zhang 08/30/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Erik Trauschke 08/30/2013

[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed

Shawn Walker 08/23/2013
 
 
Close
loading
Please Confirm
Close