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

Refactor to replace old logging to avoid mangling #3677

Merged
merged 1 commit into from May 24, 2018

Conversation

Projects
None yet
5 participants
@SanketDG
Contributor

SanketDG commented Feb 25, 2018

Fixes #3665

def _log(self, msg, level='info'):
logger = getattr(log, level)
logger(constants.LOG_TEMPLATE
def _log(self, msg):

This comment has been minimized.

@stsewd

stsewd Feb 26, 2018

Member

I think the original issue means to remove this method and do each call directly to log.info.

This comment has been minimized.

@SanketDG

SanketDG Feb 26, 2018

Contributor

Hmm, apologies but I am confused about this. The issue's primary concern seems to be an external logger mangling with the built-in one. This fixes this.

Inline-ing means, it would look like:

log.info(constants.LOG_TEMPLATE.format(project=self.project.slug, version='',msg=msg))

Why not use a helper method instead?

This comment has been minimized.

@agjohnson

agjohnson Feb 27, 2018

Contributor

When we log with a helper method, all of our logging messages reference the file/lineno location of the _log method, not where we actually called the log method from.

This comment has been minimized.

@SanketDG

SanketDG Feb 27, 2018

Contributor

@agjohnson @stsewd Ah I see, I have made the necessary changes.

@SanketDG

This comment has been minimized.

Contributor

SanketDG commented Feb 27, 2018

I have not changed other files, because I want to make sure this is the pattern to cover for other files too.

Let me know, thanks.

@@ -152,7 +146,9 @@ def symlink_cnames(self, domain=None):
else:
domains = Domain.objects.filter(project=self.project)
for dom in domains:
self._log(u"Symlinking CNAME: {0} -> {1}".format(dom.domain, self.project.slug))
log_msg = u"Symlinking CNAME: {0} -> {1}".format(dom.domain, self.project.slug)

This comment has been minimized.

@stsewd

stsewd Feb 27, 2018

Member

Use single quotes ' on literal strings, also we could get rid of the u by importing the unicode_literals or run isort to do it automatically :)

Travis is falling because of the linter, you can run the linter locally withtox -e py27-lint

This comment has been minimized.

@stsewd

stsewd Feb 27, 2018

Member

And please, could you don't touch the readthedocs/doc_builder/environments.py file? I'm taking care of the logging issue while refactoring on #3687, thanks!

@SanketDG

This comment has been minimized.

Contributor

SanketDG commented Feb 27, 2018

tox -e py27-lint seems to be passing for me, I am not sure what's wrong with the CI

@SanketDG

This comment has been minimized.

Contributor

SanketDG commented Mar 1, 2018

Ping

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 1, 2018

@SanketDG here you can see the lint error on travis https://travis-ci.org/rtfd/readthedocs.org/jobs/346909685#L813. The continuation line is bad indented.

@SanketDG

This comment has been minimized.

Contributor

SanketDG commented Mar 7, 2018

All tests have passed now. Need further review.

@@ -164,7 +160,9 @@ def symlink_cnames(self, domain=None):
def remove_symlink_cname(self, domain):
"""Remove CNAME symlink."""
self._log(u"Removing symlink for CNAME {0}".format(domain.domain))
log_msg = "Removing symlink for CNAME {0}".format(domain.domain)

This comment has been minimized.

@stsewd

stsewd Mar 7, 2018

Member

Use single quotes on all new code to keeping consistency.

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

Looks good, thanks!

@ericholscher ericholscher merged commit 57f1d08 into rtfd:master May 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@SanketDG SanketDG deleted the SanketDG:refactor_log branch Aug 16, 2018

@stsewd stsewd referenced this pull request Sep 25, 2018

Merged

Clean up logging #4665

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