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

[Bug 823495] Add MLT error logging. #1041

Closed
wants to merge 1 commit into from

Conversation

mythmon
Copy link
Contributor

@mythmon mythmon commented Jan 12, 2013

This does little more than actually record the errors that elasticutils spits out.

r?

This does little more than actually record the errors that elasticutils
spits out.
@@ -534,6 +538,7 @@ def related_documents(self):
key = 'wiki_document:related_docs:%s' % self.id
documents = cache.get(key)
if documents:
log.info('Getting MLT for {doc} from cache.'.format(doc=repr(self)))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: This should probably be a DEBUG-level thing.

@willkg
Copy link
Member

willkg commented Jan 12, 2013

That one nit is my only minor issue. Otherwise this looks good.

Also, this brings to mind whether we should switch to using .format across the board instead of %. The two problems with .format are these:

  1. It introduces a different syntax for variable replacement for localizers, so that might be an issue for using this with localized strings. We could probably check with Milos or Stas about whether this is an issue or not with Verbatim and what localizers expect/know.
  2. python 2.6 doesn't support {} in .format strings--only python 2.7 does--so we'd need to be careful about that.

Anyhow, other than that, r+.

@rlr
Copy link
Contributor

rlr commented Jan 13, 2013

cool! what @willkg said.

@mythmon
Copy link
Contributor Author

mythmon commented Jan 14, 2013

Switching this to DEBUG and landing.

Regarding .format vs %, I think that .format makes more sense for localization, because 'Hello, {name}' has more context than 'Hello, %s'. For this reason, I also think we should use name-based instead of index-based formatting. This is similar to how we use {{ jinja_variables }} in templates, and localizers have handled that fine.

@mythmon
Copy link
Contributor Author

mythmon commented Jan 14, 2013

4d7583d

@mythmon mythmon closed this Jan 14, 2013
@willkg
Copy link
Member

willkg commented Jan 14, 2013

We shouldn't be doing 'Hello, %s.' It should be 'Hello, %(name)s.'. Ick.

Having said that, I just checked the docs and they actually say to use .format with kwargs. So, nix my concerns--we should be doing that:

https://kitsune.readthedocs.org/en/latest/localization.html#making-strings-localizable

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

Successfully merging this pull request may close these issues.

3 participants