On 08/19/13 02:07 PM, Yiteng Zhang wrote:Hi Erik,
On 08/15/13 01:11 PM, Erik Trauschke wrote:
On 08/15/13 11:16 AM, Yiteng Zhang wrote:
The bug is at
My code review can be seen:
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
Thanks for you advice. My new code review can be seen at:
To demonstrate my modifications, please take a look at the following
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
pkgrepo Y pkgrepo list -F tsv -s
[repo] pkgrepo list -F json -s [repo]
pkgsign Y pkgsign -s [path_or_uri]
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.
This looks pretty good so far. I found two more places where the output should be converted: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>".
$ pkgrepo list -s /ptah/tp/repo
$ pkg history -l Code comments: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.
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)
1117++I find that self.published is a list of strings.. but I will try to replace the ugly no_build function as you said.
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
not correctly indented. Should be 4 spaces to the right.
self.published is a list of FMRIs. So I would just parse them as such and then use get_fmri(include_build=False).
tests/cli/t_pkgrepo.pyAt this moment, should I skip pkglint first?
There are still "version" members in the dicts defined in this line which contain the build release. Is this expected?
[pkg-discuss] Re: Review 16851082 - build_release (aka build version) should not be displayed