Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] [Feature] Add some basic logging around request/response process. #511

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

samLozier
Copy link

Currently, problematic queries are difficult to debug, this should make slightly it easier.

WHY are these changes introduced?

PR is opened to address this discussion: #500

Fixes #500

Context about the problem that’s being addressed:

Adds some very rudimentary debug level logging in the session and base files.

Further discussion:

  • I used f-strings, however I noticed that tox is still running tests for 2.7 and <3.7 all of which are EOL or will be shortly. Can we drop that support and move to currently supported python versions? If old python needs to be supported, I can switch them to something pre f-string.
  • this is flagged as a WIP because I know people will probably have strong feeling around the verbosity and format of the logs as well as the method for toggling them. We could use a -v flag/verbosity kwarg, the standard logging conventions, or really anything else that makes sense.

Checklist

  • I have updated the CHANGELOG (if applicable)
  • I have followed the Shopify Python guide

…rrently problematic queries are difficult to debug, this should make it easier.
@ghost ghost added the cla-needed label Apr 14, 2021
@mllemango
Copy link
Contributor

A thought: pyactiveresource has its own logging, perhaps we can take advantage of that instead of creating our own log statements?

@samLozier
Copy link
Author

@mllemango Thanks, I didn't realize that and will take a look. I suppose the question might be moot if I end up using the pyactiveresource logging, but any thoughts on the <3.7 compatibility question?

@mllemango
Copy link
Contributor

Oh yes, sorry, fully support dropping pre 3.7 compatibility

@@ -23,10 +24,14 @@ def __init__(self, site, user=None, password=None, timeout=None, format=formats.
def _open(self, *args, **kwargs):
self.response = None
try:
log.debug(f"Request: {args, kwargs}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @samLozier, I think it's better to use different patterns when you do log messages:
log.debug("Request: %s, %s", args, kwargs).
Here is an article on that: https://google.github.io/styleguide/pyguide.html#3101-logging

response = urllib.request.urlopen(request)
logging.debug(f"Response: {response}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to use log.debug(...) here instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate Kwargs before and possibly after passing them to Shopify
3 participants