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

Refactor to replace old logging to avoid mangling #3677

Merged
merged 1 commit into from May 24, 2018
Merged

Refactor to replace old logging to avoid mangling #3677

merged 1 commit into from May 24, 2018

Conversation

@SanketDG
Copy link
Contributor

@SanketDG 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):
Copy link
Member

@stsewd stsewd Feb 26, 2018

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

Copy link
Contributor Author

@SanketDG SanketDG Feb 26, 2018

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?

Copy link
Contributor

@agjohnson agjohnson Feb 27, 2018

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.

Copy link
Contributor Author

@SanketDG SanketDG Feb 27, 2018

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

@SanketDG
Copy link
Contributor Author

@SanketDG 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)
Copy link
Member

@stsewd stsewd Feb 27, 2018

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

Copy link
Member

@stsewd stsewd Feb 27, 2018

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
Copy link
Contributor Author

@SanketDG 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
Copy link
Contributor Author

@SanketDG SanketDG commented Mar 1, 2018

Ping

@stsewd
Copy link
Member

@stsewd 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
Copy link
Contributor Author

@SanketDG 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)
Copy link
Member

@stsewd stsewd Mar 7, 2018

Use single quotes on all new code to keeping consistency.

@ericholscher
Copy link
Member

@ericholscher ericholscher commented May 24, 2018

Looks good, thanks!

@ericholscher ericholscher merged commit 57f1d08 into readthedocs:master May 24, 2018
1 check passed
@SanketDG SanketDG deleted the refactor_log branch Aug 16, 2018
@stsewd stsewd mentioned this pull request Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants