Skip to main content

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

  • From: Yiteng Zhang < >
  • To:
  • Cc: erik Trauschke < >
  • Subject: [pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed
  • Date: Tue, 20 Aug 2013 17:26:07 -0700

On 08/20/13 03:33 PM, Erik Trauschke wrote:
On 08/19/13 02:07 PM, Yiteng Zhang wrote:
On 08/15/13 01:11 PM, Erik Trauschke wrote:


On 08/15/13 11:16 AM, Yiteng Zhang wrote:
Hi,

The bug is at
https://bug.oraclecorp.com/pls/bug/webbug_print.show?c_rptno=16851082

My code review can be seen:

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

In principle this goes into the right direction but there is still way
more code to be converted.

For instance, 'pkg install -v <pkg>' will show a list of FMRIs which
are going to change. The code for this is in __display_plan().
And there are probably a few other places where the output need changing.

I would go and look at how the display functions (in the files you
already changed) retrieve their FMRIs and then look where else the
same method of retrieval is used.

You can also have a look in the test suite and check which pkg
applications get tested for an expected output that includes the build
version.

Erik
Hi,
Thanks for you advice. My new code review can be seen at:
http://ips.java.net/webrev/yitezhan/16851082_2/

To demonstrate my modifications, please take a look at the following
details.

To omit the build_release of the FMRI displays, I have modified the
output of some of the pkg utilities command:

Command    Changed?(Y or N)     Detail
pkg                          Y                   pkg list -v pkg info
pkg install --licenses    pkg install -v    pkg uninstall -v
pkgdepend              N
pkgrepo                   Y                   pkgrepo list -F tsv -s
[repo]          pkgrepo list -F json -s [repo]
pkgdiff                      N
pkgfmt                     N
pkglint                      N
pkgmerge                N
pkgmogrify              N
pkgsurf                   N
pkgsend                  N
pkgsign                   Y                    pkgsign -s [path_or_uri]
packagemanager    N
pkgrecv                   Y                   pkgrecv -s [src_repo_uri]
--newest        pkgrecv --clone

The reason why I didn't change some of them is because the commands do
not take the FMRIs for displays.
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>".
$ 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.
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.
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?
Yiteng


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

Yiteng Zhang 08/15/2013

[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
 
 
Close
loading
Please Confirm
Close