Large portions of the patch was automatically created with autopep8: find base/ -name '*.py' | xargs autopep8 --in-place --ignore E309 \ --aggressive find base/common/upgrade base/server/upgrade -type f -and \ -not -name .gitignore | autopep8 --in-place --ignore E309 --aggressive autopep8 --in-place --ignore E309 --aggressive \ base/common/sbin/pki-upgrade \ base/server/sbin/pkispawn \ base/server/sbin/pkidestroy \ base/server/sbin/pki-server \ base/server/sbin/pki-server-upgrade About two dozent violations were fixed manually. https://fedorahosted.org/pki/ticket/708 NOTE: I'm going to add some checks to the PKI core spec file and tox later.
On 8/11/2015 2:31 PM, Christian Heimes wrote:
Large portions of the patch was automatically created with autopep8:
find base/ -name '*.py' | xargs autopep8 --in-place --ignore E309 \ --aggressive find base/common/upgrade base/server/upgrade -type f -and \ -not -name .gitignore | autopep8 --in-place --ignore E309 --aggressive autopep8 --in-place --ignore E309 --aggressive \ base/common/sbin/pki-upgrade \ base/server/sbin/pkispawn \ base/server/sbin/pkidestroy \ base/server/sbin/pki-server \ base/server/sbin/pki-server-upgrade
About two dozent violations were fixed manually.
https://fedorahosted.org/pki/ticket/708
NOTE: I'm going to add some checks to the PKI core spec file and tox later.
Some comments: 1. In base/common/python/pki/cli.py we probably shouldn't insert a line break inside an argument: # If module command exists, include it as arguments: <module # command> <args>... 2. In base/common/python/pki/crypto.py it probably should have been a decorator, but this is a separate issue. # abc.abstractmethod def get_cert(self, cert_nick): 3. In base/kra/functional/drmclient_deprecated.py this code is probably less readable than the original. Is there a better way to format it? It's a deprecated code too, maybe we can just ignore/remove it. response['cert'] = b64.replace( CERT_HEADER, "").replace( CERT_FOOTER, "") 4. In base/server/python/pki/server/cli/migrate.py the line break isn't very nice: if valve.get( 'className') == 'org.apache.catalina.valves.AccessLogValve': Everything else looks good. It's up to you how you want to address the above items. Regardless, it's ACKed. -- Endi S. Dewata
On 2015-08-13 23:47, Endi Sukma Dewata wrote:
Some comments:
1. In base/common/python/pki/cli.py we probably shouldn't insert a line break inside an argument:
# If module command exists, include it as arguments: <module # command> <args>...
2. In base/common/python/pki/crypto.py it probably should have been a decorator, but this is a separate issue.
# abc.abstractmethod def get_cert(self, cert_nick):
3. In base/kra/functional/drmclient_deprecated.py this code is probably less readable than the original. Is there a better way to format it? It's a deprecated code too, maybe we can just ignore/remove it.
response['cert'] = b64.replace( CERT_HEADER, "").replace( CERT_FOOTER, "")
4. In base/server/python/pki/server/cli/migrate.py the line break isn't very nice:
if valve.get( 'className') == 'org.apache.catalina.valves.AccessLogValve':
Everything else looks good. It's up to you how you want to address the above items. Regardless, it's ACKed.
Hi Endi, thanks for the review. The automatic code wrapping feature of autopep8 isn't perfect. Before I created the patch ball, I fixed a bunch of places manually. It looks like three places slipped through. Good work :) I rearranged 1, 3 and 4 and made the code more readable before I pushed the patch to master. Pushed to master in 12badcabc1cd345256a4902f7b0583cf667ecd8d Regards, Christian
participants (2)
-
Christian Heimes -
Endi Sukma Dewata