Re: [gentoo-portage-dev] [PATCH] prepstrip: rewrite in python

[Alexander Berntsen]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Great idea! ANd, wow, unit tests, weee. :-) Here are a few comments,
mostly on style:

(I'm not sure how you usually do code reviews/comments here, so please
pardon my ignorance on the matter.)

pym/portage/bin/prepstrip.py:
	-You import sys, shlex, _copyxattr and xattr but never use them
	-On line 86, I would change the indentation to be
            IMPLICIT_FEATURES = frozenset((
                                           'binchecks',
                                            'strip',
                                          ))
	-I would align the following lines the same way:
		93
		134
		193
		198
		238
		388
		443
		478
		506
		652
		654
	-On line 98, you have too many newlines before the method
	-On line 175 you catch an exception "as e" but never use the 'e'
	-The following lines are > 79 chars:
		219
		484
		492
		591
		633
		634
	-The inline comment on line 486 should have two spaces
	-In Prepstrip(), why do you
		if Features.installedsources is None:
			warn
			warn
		else if Features.installedsources:
			stuff
			...
	 instead of
		if Features.installedsources is None:
			warn
			warn
			return
		stuff
		...
	 - Also, instead of
		for path, reason in stripped:
			if not reason:
				print(...)
			else:
				print(...)
	 why not use a ternary?

pym/portage/tests/test_prepstrip.py
	-You import sys but never use it
	-Align indentation:
		65
		73
		179
	- >79 chars:
		110
		156
		169
	-In the docstring for testMissing, "garbage" is usually the preferred
term, not "crap"

pym/portage/util/prallel.py:
	-Indentation is a bit WTF -- Python indentation should be a multiple
of four, and this is kind of serious since this is not a "warn" but an
actual *error* for Python linters, making it really hard to find more
serious errors when every line is "error! E111 Indentation is not a
multiple of four"
	-On line 141 you do results = [], but never use results
	-Line 268 you catch an exception as ex but never use ex
	-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
	- >79 chars:
		325
		334
		443
		557

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.
- -- 
Alexander
alexander@××××××.net
http://plaimi.net/~alexander
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iF4EAREIAAYFAlJfskUACgkQRtClrXBQc7XfiQD/VUhtH8Nc1RmNmLYmDzXBHSGS
0S8SMrEka+1cp+BHkHkA/3zOrsoAWra4Ynux2WCOpNYvLsDo0AghGEz8X7oYNEtn
=7rF9
-----END PGP SIGNATURE-----