Skip to content
This repository was archived by the owner on Oct 6, 2026. It is now read-only.

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

Closed
samLozier wants to merge 1 commit into
Shopify:mainfrom
samLozier:RequestResponse_Logging
Closed

samLozier wants to merge 1 commit into
Shopify:mainfrom
samLozier:RequestResponse_Logging

Conversation

@samLozier

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Oh yes, sorry, fully support dropping pre 3.7 compatibility

Comment thread shopify/base.py
def _open(self, *args, **kwargs):
self.response = None
try:
log.debug(f"Request: {args, kwargs}")

Copy link
Copy Markdown

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

Comment thread shopify/session.py
request = urllib.request.Request(url, urllib.parse.urlencode(query_params).encode("utf-8"))
log.debug(f"Request URL: {url}, query params: {query_params}")
response = urllib.request.urlopen(request)
logging.debug(f"Response: {response}")

Copy link
Copy Markdown

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.

@Suleimanlatrsh

Copy link
Copy Markdown
Contributor

Thanks for contributing! This package is deprecated and we're archiving this repo, so we're closing all open issues and PRs. Please use shopify-app-python instead, and open new issues there. More context: Rethinking support for PHP/Python packages

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate Kwargs before and possibly after passing them to Shopify

4 participants