Fixes from linters (and some cosmetics) #246
Conversation
socketpair
changed the title
| DEFAULT_DIRECTORY_URL = "https://acme-v02.api.letsencrypt.org/directory" | ||
|
|
||
| LOGGER = logging.getLogger(__name__) | ||
| LOGGER.addHandler(logging.StreamHandler()) |
There was a problem hiding this comment.
Adding handlers spoils logic in applications that use this file as a library.
| @@ -23,28 +22,28 @@ def _b64(b): | |||
| # helper function - run external commands | |||
| def _cmd(cmd_list, stdin=None, cmd_input=None, err_msg="Command Line Error"): | |||
| proc = subprocess.Popen(cmd_list, stdin=stdin, stdout=subprocess.PIPE, stderr=subprocess.PIPE) | |||
| out, err = proc.communicate(cmd_input) | |||
| stdout, err = proc.communicate(cmd_input) | |||
| time.sleep(0 if result is None else 2) | ||
| result, _, _ = _send_signed_request(url, None, err_msg) | ||
| return result | ||
|
|
||
| # parse account key to get public key | ||
| log.info("Parsing account key...") |
There was a problem hiding this comment.
ellipses and exclamation marks are not a good style in logging messages.
| if subject_alt_names is not None: | ||
| for san in subject_alt_names.group(1).split(", "): | ||
| if san.startswith("DNS:"): | ||
| domains.add(san[4:]) | ||
| log.info("Found domains: {0}".format(", ".join(domains))) |
There was a problem hiding this comment.
.format() formatting in logging is a bad style, at least because formatting occurs even if (according to loglevel) message should not be logged.
There was a problem hiding this comment.
For some reason GitHub isn't showing me the additions for this line unless I look at the file directly. Just a heads up if anyone else is confused by the seemingly broken change.
| if contact is not None: | ||
| account, _, _ = _send_signed_request(acct_headers['Location'], {"contact": contact}, "Error updating contact details") | ||
| log.info("Updated contact details:\n{0}".format("\n".join(account['contact']))) | ||
| log.info("Updated contact details: %s.", "; ".join(account['contact'])) |
There was a problem hiding this comment.
newlines in logging may not work in syslog.
| @@ -136,8 +135,8 @@ def _poll_until_not(url, pending_statuses, err_msg): | |||
| wellknown_file.write(keyauthorization) | |||
|
|
|||
| # check that the file is in place | |||
| wellknown_url = "http://{0}/.well-known/acme-challenge/{1}".format(domain, token) | |||
There was a problem hiding this comment.
According to old code, exception theoretically may happen in .format(). In this case code in the except block will use undefined variable.
There was a problem hiding this comment.
More specifically, it could raise a ValueError, which would be mistakenly caught.
| parser.add_argument("--disable-check", default=False, action="store_true", help="disable checking if the challenge file is hosted correctly before telling the CA") | ||
| parser.add_argument("--directory-url", default=DEFAULT_DIRECTORY_URL, help="certificate authority directory url, default is Let's Encrypt") | ||
| parser.add_argument("--ca", default=DEFAULT_CA, help="DEPRECATED! USE --directory-url INSTEAD!") | ||
| parser.add_argument("--contact", metavar="CONTACT", default=None, nargs="*", help="Contact details (e.g. mailto:[email protected]) for your account-key") | ||
|
|
||
| args = parser.parse_args(argv) | ||
| LOGGER.setLevel(args.quiet or LOGGER.level) | ||
| logging.basicConfig(level=logging.ERROR if args.quiet else logging.INFO) |
There was a problem hiding this comment.
instead of adding handlers by hand.
| signed_crt = get_crt(args.account_key, args.csr, args.acme_dir, log=LOGGER, CA=args.ca, disable_check=args.disable_check, directory_url=args.directory_url, contact=args.contact) | ||
| sys.stdout.write(signed_crt) | ||
|
|
||
| if __name__ == "__main__": # pragma: no cover | ||
|
|
||
| if __name__ == "__main__": # pragma: no cover |
There was a problem hiding this comment.
Yep, 200 lines limit still honored.
| @@ -1,20 +1,19 @@ | |||
| #!/usr/bin/env python | |||
| # Copyright Daniel Roesler, under MIT license, see LICENSE at github.com/diafygi/acme-tiny | |||
| import argparse, subprocess, json, os, sys, base64, binascii, time, hashlib, re, copy, textwrap, logging | |||
acme_tiny.py
Outdated
| except ImportError: | ||
| from urllib2 import urlopen, Request # Python 2 | ||
| import argparse, subprocess, json, os, sys, base64, binascii, time, hashlib, re, textwrap, logging | ||
| if sys.version_info[0] >= 3: |
There was a problem hiding this comment.
much better than try...except. And also self-documenting. so I removed the comments.
There was a problem hiding this comment.
This is a bit of an anti-pattern, as you shouldn't rely on the versioning information to predict the standard library API. That said, the existing solution has its own issues. A better option would be using future.standard_library.hooks, which would also remove the redundancies.
There was a problem hiding this comment.
Wouldn't that require installing a non-stdlib module? https://pypi.org/project/future/
It isn't much of an anti-pattern if at all, though it is a bit better as far as self-documenting goes.
There was a problem hiding this comment.
For me, I would remove Python2 support at all.
btw, should I revert changing this chunk ?
There was a problem hiding this comment.
Ack, crud, yeah.
I agree that it's more self-documenting, but it also relies on an assumed association between the value of sys.version_info and the layout of the API. You can rely on that in current versions of CPython, but there's no guarantee of that being the case in other runtimes or in the future.
There was a problem hiding this comment.
Ok, I will revert this chunk.
P.S.
I'm just curious. I'm not a native English speaker. How would you explain "Ack, crud, yeah." ? Google tells me something weird.
There was a problem hiding this comment.
For me, I would remove Python2 support at all.
Me too. Would save a couple lines. :-)
There was a problem hiding this comment.
"Ack, crud, yeah." = Oops. Yes, you're correct.
Unfortunately, Python 2 support needs to remain in place for now, as it is still the default on some OSes.
socketpair
changed the title
|
Should I change anything in order you to merge this PR ? |
|
BUMP |
|
Now that we're using github actions, I'd be interested in adding a linter workflow. What linter was used here? |
No description provided.