Skip to content

Conversation

@jimabramson
Copy link

....

JIRA: FOR-398, FOR-399

@gwprice
@nedbat

Copy link
Contributor

Choose a reason for hiding this comment

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

This is doing a bunch of work to get one string. It's uglier but an if/elif ladder would also work.

Copy link

Choose a reason for hiding this comment

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

It would be better to use ugettext_noop on the constant definitions and then ugettext where these are used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea.

@gwprice
Copy link

gwprice commented Jan 14, 2014

This needs to be rebased. Also, there are at least a few missing strings (see #1793); you should run through one more time to make sure you caught everything.

@jimabramson
Copy link
Author

updated:

  • switched to ugettext_noop in django_comment_common/models.py per suggestions
  • revisited comment/thread count display
  • rebased and resolved conflicts
  • rechecked per suggestion and found 2 or 3 more placeholder="" i'd missed. (wish i'd known about https://github.com/edx/edx-platform/pull/1793 before, would have saved some work.)

Copy link

Choose a reason for hiding this comment

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

ugettext is unused

@jimabramson
Copy link
Author

updated with unused ugettext import in django_comment_common.models removed

@gwprice
Copy link

gwprice commented Jan 14, 2014

👍

1 similar comment
@nedbat
Copy link
Contributor

nedbat commented Jan 15, 2014

👍

jimabramson pushed a commit that referenced this pull request Jan 15, 2014
i18n: externalize strings in discussion forums templates and python code...
@jimabramson jimabramson merged commit 0b617fc into master Jan 15, 2014
@jzoldak jzoldak deleted the feature/jsa/forums-i18n-mako branch May 5, 2014 14:55
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.

4 participants