Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(4)

#18128: pygettext: non-standard timestamp format in POT-Creation-Date

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 9 months ago by jwilk
Modified:
4 years, 9 months ago
Reviewers:
os.urandom
CC:
barry, jwilk_jwilk.net, r.david.murray, devnull_psf.upfronthosting.co.za, flipmcf_gmail.com
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/test/test_tools/test_i18n.py View 1 1 chunk +75 lines, -0 lines 13 comments Download
Tools/i18n/pygettext.py View 1 1 chunk +1 line, -3 lines 0 comments Download

Messages

Total messages: 1
blackfawn
4 years, 9 months ago #1
http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n.py
File Lib/test/test_tools/test_i18n.py (right):

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:10: class Test_pygettext(unittest.TestCase):
Rename the class GettextTests or PygettextTests to fit the naming style of other
tools tests.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:13: script = os.path.join(toolsdir,'i18n',
'pygettext.py')
Space after the first comma.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:15: def getHeader(self, data):
"get" implies a very fast+simple operation. A better name for this would be
parseHeader or readHeader.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:15: def getHeader(self, data):
Maybe also add a test to verify the header format? These tests will pass even if
there are no "" quotes around the header fields, or no \\n separators.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:15: def getHeader(self, data):
Keep a consistent naming style for your methods, i.e., this should be
get_header.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:16: """ utility: return the header of a .po
file as a dictionary """
No spaces at the start/end of the docstring.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:19: 
No blank lines in the beginning of blocks.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:36: with temp_cwd(None) as cwd:
It would be nicer to have this temp_cwd as part of the setUp (with a respective
addCleanup), so the entire test method doesn't have to be inside a "with" block.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:50: self.assertIn("Generated-By", header)
Pretty sure this field is optional. If you're checking it at all, go all the way
and make sure it says "pygettext.py something something".

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:52: #not sure if these should be required in
POT (template) files
Language is definitely optional. I'm almost certain the other one is too.

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:59: def test_POT_Creation_Date(self):
Don't capitalize "creation" and "date".

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:70: #peel off the escaped newline at the end of
string
Maybe do this in getHeader?

http://bugs.python.org/review/18128/diff/14606/Lib/test/test_tools/test_i18n....
Lib/test/test_tools/test_i18n.py:74: #if this throws an exception: you fail.
This is true for every line of code in every unittest method. No need to make an
explicit comment.
It would be nice to add some assertions to check the resulting datetime object,
but if you don't, rename this method so it'll be clear it's only testing the
*format* of the creation date.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+