On Thursday 17 October 2013 05:47:49 Alexander Berntsen wrote: > pym/portage/bin/prepstrip.py: > -You import sys, shlex, _copyxattr and xattr but never use them
i've run a linter on the files now
> -On line 86, I would change the indentation to be
i think existing style is correct. it's certainly more
readable/maintainable/mutable.
> -On line 98, you have too many newlines before the method
fixed
> -On line 175 you catch an exception "as e" but never use the 'e'
fixed
> -The following lines are > 79 chars:
i've run a linter on the files now
> -The inline comment on line 486 should have two spaces
fixed
> -In Prepstrip(), why do you
i like the existing style better rather than returning early. makes
adding/removing sections cleaner (which i've done during development).
> - Also, instead of
> for path, reason in stripped:
> if not reason:
> print(...)
> else:
> print(...)
> why not use a ternary?
inlining formats/tuples with a ternary doesn't seem like it'd really be
cleaner.
print((' %-*s # %s' % (align, path, reason)) if reason
else (' %s' % path), file=out)
that's pretty busy and not easy to track.
> pym/portage/util/prallel.py:
as the top of the file documents, this is imported from an external tree. i'm
not making changes here in general. for python3 compat, i'm merging those to
the upstream copy.
stylewise, it's as expected
> -Background.Wait(): almost the whole thing is in a Try, which is a
> bit ehh, and if you don't want to rewrite that than maybe at least
> either remove the finally or put the return inside the finally
it is a bit hairy, but it's making a hairy system work. it's making sure
output is captured in parallel and replied sanely rather than interleaving it
all. similar to handling of exceptions.
> Oh, and you use camelCase for functions and methods. In python we use
> underscores. I don't know if there's some convention outside of emerge
> (which is the part of Portage I'm most familiar with) to not use
> underscore. If not, I would prefer it if we were consistent and used
> underscores everywhere.
i'll fix the new code
-mike
signature.asc
Description: This is a digitally signed message part.
