On 08/09/13 18:54, Yiteng Zhang wrote:Hi Shawn,
On 08/ 9/13 02:32 PM, Shawn Walker wrote:
On 08/09/13 12:54, Yiteng Zhang wrote:Hi,
I just fixed thsi bug and push my code review to the webrev. Please
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 expect to
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
Yes, I meant output_format == "default"; sorry about that.
the t_pkg_varcet.py in order to pass the test cases. My code review can
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.
[pkg-discuss] Re: Code review request for Bug 16986557 - prefixes should be omitted from output of facet and variant