On Sat, 17 Nov 2012 11:58:48 -0500 Mike Gilbert <flop...@gentoo.org> wrote:
> On Fri, Nov 16, 2012 at 1:44 PM, Michał Górny <mgo...@gentoo.org> wrote:
> > This is much more complex than the previous version.
> >
>
> The inline comments and debug-print statements are helpful.
>
> Do we have a way to test this with various combinations of
> PYTHON_COMPAT, PYTHON_TARGETS, eselect python, and USE_PYTHON?
Well, I've just tested it by hand. If someone wants to work on bringing
tests for it, I don't mind. To be honest, I don't really feel like they
would be of much use. I tested and fixed what came to my head. I doubt
tests will be able to find much (especially if I write them); live
testing will be probably much more useful.
The warning code is imperfect and mostly transitional. It tries to help
but can't do more than PMS permits it to. It should lie there for some
time, then we should be able to get rid of it, and forget about
the whole thing. Considering that, writing tests is IMO mostly a waste
of time.
> I have some suggestions on the actual warning text below.
>
> > + if [[ ${warned} ]]; then
> > + ewarn
> > + ewarn "Please note that after switching the
> > active Python interpreter,"
> > + ewarn "you may need to run 'python-updater'
> > to ensure the system integrity."
>
> Change "ensure the system integrity" to something like "rebuild
> affected packages". The former statement is awkward and doesn't really
> explain anything.
Applied.
> > + if [[ ${USE_PYTHON} ]]; then
> > + ewarn "It seems that your USE_PYTHON
> > setting does list different Python"
>
> Drop "It seems that"; it softens the statement for no reason. This
> also applies to a few statements below.
Well, the main reason for that was to emphasis that the check
is imperfect and the result simply may be wrong. How can I express that
without adding yet another paragraph to the lengthy enough text?
> The phrase "does list" looks odd in English.
Right.
> Replace with "Your USE_PYTHON setting lists different Python ...".
>
> > + ewarn "implementations than your
> > PYTHON_TARGETS variable. Please consider"
> > + ewarn "using the following value instead:"
> > + ewarn
> > + ewarn "
> > USE_PYTHON='\033[35m${old[@]}${new[@]+ \033[1m${new[@]}}\033[0m'"
> > +
> > + if [[ ${removed[@]} ]]; then
> > + ewarn
> > + ewarn "(removed
> > \033[31m${removed[@]}\033[0m)"
> > + fi
> > + else
> > + ewarn "It seems that you need to set
> > USE_PYTHON to make sure that legacy"
>
> s/It seems that you need/You should/
The same as above. Two 'it seems' with different values vs 'you should'
with two different values.
>
> > + ewarn "packages will be built with respect
> > to PYTHON_TARGETS correctly:"
> > + ewarn
> > + ewarn "
> > USE_PYTHON='\033[35;1m${new[@]}\033[0m'"
> > + fi
> > +
> > + ewarn
> > + ewarn "Please note that after changing the
> > USE_PYTHON variable, you may need"
> > + ewarn "to run 'python-updater' to ensure the system
> > integrity."
>
> Again, s/ensure the system integrity/rebuild affected packages/.
Applied.
--
Best regards,
Michał Górny
signature.asc
Description: PGP signature
