Fixes from linters (and some cosmetics) #246

socketpair · 2020-04-09T18:09:47Z

No description provided.

socketpair · 2020-04-09T18:10:48Z

acme_tiny.py

 DEFAULT_DIRECTORY_URL = "https://acme-v02.api.letsencrypt.org/directory"

 LOGGER = logging.getLogger(__name__)
-LOGGER.addHandler(logging.StreamHandler())


Adding handlers spoils logic in applications that use this file as a library.

socketpair · 2020-04-09T18:11:36Z

acme_tiny.py

@@ -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)


shadows upper variable

socketpair · 2020-04-09T18:12:57Z

acme_tiny.py

            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...")


ellipses and exclamation marks are not a good style in logging messages.

socketpair · 2020-04-09T18:14:50Z

acme_tiny.py

    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)))


.format() formatting in logging is a bad style, at least because formatting occurs even if (according to loglevel) message should not be logged.

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.

socketpair · 2020-04-09T18:15:41Z

acme_tiny.py

    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']))


newlines in logging may not work in syslog.

socketpair · 2020-04-09T18:17:37Z

acme_tiny.py

@@ -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)


According to old code, exception theoretically may happen in .format(). In this case code in the except block will use undefined variable.

More specifically, it could raise a ValueError, which would be mistakenly caught.

socketpair · 2020-04-09T18:19:28Z

acme_tiny.py

    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)


instead of adding handlers by hand.

socketpair · 2020-04-09T18:20:07Z

acme_tiny.py

    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


Yep, 200 lines limit still honored.

socketpair · 2020-04-09T18:20:31Z

acme_tiny.py

@@ -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


copy is not used.

socketpair · 2020-04-09T18:21:00Z

acme_tiny.py

-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:


much better than try...except. And also self-documenting. so I removed the comments.

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.

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.

For me, I would remove Python2 support at all.

btw, should I revert changing this chunk ?

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.

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.

For me, I would remove Python2 support at all.

Me too. Would save a couple lines. :-)

"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 · 2020-04-09T22:32:53Z

Should I change anything in order you to merge this PR ?

socketpair · 2020-06-25T13:23:36Z

BUMP

diafygi · 2021-08-21T20:07:49Z

Now that we're using github actions, I'd be interested in adding a linter workflow. What linter was used here?

socketpair added 2 commits April 9, 2020 22:50

Loggers fix

9308db4

Fix logging messages

a6c8768

socketpair changed the title ~~Fixes from linters~~ Fixes from linters (and some consmetics) Apr 9, 2020

socketpair commented Apr 9, 2020

View reviewed changes

Linters fixes

82f8b3f

socketpair changed the title ~~Fixes from linters (and some consmetics)~~ Fixes from linters (and some cosmetics) Apr 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes from linters (and some cosmetics) #246

Fixes from linters (and some cosmetics) #246

socketpair commented Apr 9, 2020

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020

socketpair Apr 9, 2020

socketpair Apr 9, 2020

rspeed Apr 9, 2020

socketpair Apr 9, 2020

socketpair Apr 9, 2020

rspeed Apr 9, 2020

socketpair Apr 9, 2020

socketpair Apr 9, 2020

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020 •

edited

Loading

rspeed Apr 9, 2020

StephenBrown2 Apr 9, 2020

socketpair Apr 9, 2020 •

edited

Loading

rspeed Apr 9, 2020

socketpair Apr 9, 2020 •

edited

Loading

StephenBrown2 Apr 9, 2020

rspeed Apr 9, 2020

socketpair commented Apr 9, 2020

socketpair commented Jun 25, 2020

diafygi commented Aug 21, 2021

Fixes from linters (and some cosmetics) #246

Are you sure you want to change the base?

Fixes from linters (and some cosmetics) #246

Conversation

socketpair commented Apr 9, 2020

socketpair Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

socketpair Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socketpair commented Apr 9, 2020

socketpair commented Jun 25, 2020

diafygi commented Aug 21, 2021

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020 •

edited

Loading

socketpair Apr 9, 2020 •

edited

Loading