Skip to main content

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

  • From: Erik Trauschke < >
  • To: Yiteng Zhang < >
  • Cc:
  • Subject: [pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed
  • Date: Wed, 21 Aug 2013 08:58:53 -0700



On 08/20/13 05:26 PM, Yiteng Zhang wrote:
On 08/20/13 03:33 PM, Erik Trauschke wrote:
Hi Erik,
Thanks for your comments.
This looks pretty good so far. I found two more places where the
output should be converted:
$ pkgrepo list -s /ptah/tp/repo
It's the output of VERSION. I will change it to just keep the release
and branch version, like the output as "pkg list <pkg>".

Thanks.

$ pkg history -l Code comments:
src/client.py:
535++
    Even though that works it's not ideal. If the version format ever
changes, you'll have to change this line too. Better would be just to
parse the string into a Version object and then use get_version():
    v = version.Version(ver, None)
    sver = v.get_version(include_build=False)

Yes, I think either way has its advantage. Shawn has talked about it
that users would expect "pkg list -v" not spending too much time. So
here I just call a function instead of creating an object.

I talked to Shawn about that. It's done for performance reasons as you said. Maybe add a comment so people know why you're doing this.

1117++
    Having this as an inline function here is kinda ugly, since you
could use it in other places in client.py too. My suggestions are
1) don't use it at all, you are not saving that much code here anyway
2) define the function outside of display_plan so it's usable in any
other function as well.
3) define it in modules/misc.py. This way callers from other modules
can use it too (like the test suite)

I'm fine with either of the above

1133
    not correctly indented. Should be 4 spaces to the right.

tests/cli/t_pkgrecv.py
201++
    self.published is a list of FMRIs. So I would just parse them as
such and then use get_fmri(include_build=False).

I find that self.published is a list of strings.. but I will try to
replace the ugly no_build function as you said.

Yes, the strings are FMRIs. In the test bench we are not that concerned about performance unless it really speeds things up. In a lot of cases, faster code has worse readability because you might play tricks with the compiler or the memory. So you always have to choose if it's worth sacrificing readability for performance reasons. In an application which should run as fast as possible this might be the case, in the test bench it's like not.

Wrt the nobuild function. Here it is alright using it, just implemented differently (as stated above). At the moment it's pretty hard to figure out what it actually does.

tests/cli/t_pkgrepo.py
1393
    There are still "version" members in the dicts defined in this
line which contain the build release. Is this expected?


Erik
At this moment, should I skip pkglint first?

Looking at Tim's reply I'd say yes, skip it for now. Once we completely remove the build release we can change it.

One more thing:
In modules/fmri.py, line 363 you should not use a positional argument in a function which expects a key value pair. while that works it is prone to something going wrong when people change this code. Use include_build=include_build.

Erik


[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/15/2013

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

Yiteng Zhang 08/19/2013

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

Tim Foster 08/19/2013

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

Yiteng Zhang 08/19/2013

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

Tim Foster 08/19/2013

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

Erik Trauschke 08/19/2013

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

Tim Foster 08/19/2013

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

Shawn Walker 08/19/2013

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

Erik Trauschke 08/20/2013

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

Yiteng Zhang 08/21/2013

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

Erik Trauschke 08/21/2013

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

Yiteng Zhang 08/21/2013

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

Erik Trauschke 08/21/2013

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

Yiteng Zhang 08/21/2013

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

Erik Trauschke 08/22/2013

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

Yiteng Zhang 08/22/2013

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

Erik Trauschke 08/22/2013

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

Shawn Walker 08/22/2013

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

Yiteng Zhang 08/22/2013

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

Tim Foster 08/23/2013

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

Yiteng Zhang 08/23/2013
 
 
Close
loading
Please Confirm
Close