On 08/15/13 04:27 PM, Edward Pilatowicz wrote:
On Mon, Aug 12, 2013 at 02:20:59PM -0700, Shawn Walker wrote:Hi Edward,
On 08/12/13 10:49, Yiteng Zhang wrote:hey Yiteng,
On 08/11/13 01:40 PM, Shawn Walker wrote:As soon as you've had a few successfully reviewed changesets,
On 08/09/13 18:54, Yiteng Zhang wrote:Hi Shawn,
On 08/ 9/13 02:32 PM, Shawn Walker wrote:Yes, I meant output_format == "default"; sorry about that.
On 08/09/13 12:54, Yiteng Zhang wrote:Hi,
I just fixed thsi bug and push my code review to the webrev.
take a look on it.
One thing to mention, after I run the test scripts, it reported
some mismatches failures. It is because the test scripts still
see the output with prefixes. Thanks.
Unfortunately, this set of changes will drop the 'variant.' and
'facet.' prefixes from more than just the default output. The
original bug requests that the prefixes are only omitted from
the default output.
In other words, you should only omit the prefixes if the
output_format != "default".
Also, as a matter of style, we generally do not use '' to delimit
strings or even single characters; we use "" instead. (As seen
on lines 4800, 4801, 4811, 4812, 4890, 4891.)
You should be able to fix all of the remaining tests by simply
changing __assert_facet_matches and __assert_variant_matches in
src/tests/cli/t_pkg_varcet.py to remove the 'facet.' or 'variant.'
prefix from the exp_def string variable.
Sorry about that I misunderstood the meaning of default output.
Thanks for your advise. Now I only omit the prefixed if the
output_format == "default" (I think you mean equal). Also, I modified
the t_pkg_varcet.py in order to pass the test cases. My code review cansrc/client.py:
be seen at:
This is now correct.
line 119: We always place a space before and after operators such as
'-', '+', etc. However, read on.
lines 118-121: This could be more simply written as:
expected = "".join(
(prefix + l)
for l in expected.splitlines(True)
After you make these changes, be certain you perform a full test suite
run to ensure nothing else has broken.
You can run the test suite faster by using the -j option to have it
run tests in parallel where possible, like this:
python tests/run.py -j8
Assuming the test suite passes, you can push your changeset.
Thanks, I have made these changes and a full test suite was passed.
I wish I can push the code, but I am not authorized to write to the
someone will nominate you for commit access and you'll be able to
putback. Until then, another pkg team member can push your changes.
In this particular case, Ed is right, there's unfortunately some
logic in the kernel zone installation scripts that currently relies
on the 'facet.' prefixes that Ed and I both missed during our
previous review of the zone scripts.
Until 17301112 is fixed, we won't be able to push these changes.
So for now, just hold onto your workspace and sometime during ON
build 30, we should be able to push them.
fyi, i just pushed 17301112 into ON gate for build s12_30. so once the
pkg gate closes for s12_29 (and opens for s12_30) you should be OK to
push this fix.
I appreciate it. I will check the bug again when s12_30 opens. Thanks.
Description: Text document
[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant