Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
911563002:
Fixed bug with paragraphs in policy description (Closed)
|
Created:
5 years, 10 months ago by peletskyi Modified:
5 years, 10 months ago CC:
grit-developer_googlegroups.com Base URL:
https://chromium.googlesource.com/external/grit-i18n.git@master Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionFixed bug 437524. There are paragraphs in policy description now.
BUG=437524
R=mnissler@chromium.org
Committed: https://code.google.com/p/grit-i18n/source/detail?r=186
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #
Total comments: 1
Patch Set 4 : Change name of the variable #Patch Set 5 : #
Total comments: 8
Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Patch Set 9 : Fix for tests #Patch Set 10 : #
Messages
Total messages: 37 (12 generated)
peletskyi@chromium.org changed reviewers: + mnissler@chromium.org
Hi Mattisas, please review my changes. Best wishes, Oleksandr
https://codereview.chromium.org/911563002/diff/40001/grit/format/policy_templ... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/40001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:700: self._url_matcher = lazy_re.compile('(http://[^\\s]*[^\\s\\.])|\n\n') It's rather confusing to do both URL matching and paragraph splitting via the same regular expression and code. I'd prefer you break paragraphs in a first step and then perform link replacement in a second step.
On 2015/02/10 08:30:08, Mattias Nissler wrote: > https://codereview.chromium.org/911563002/diff/40001/grit/format/policy_templ... > File grit/format/policy_templates/writers/doc_writer.py (right): > > https://codereview.chromium.org/911563002/diff/40001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:700: self._url_matcher = > lazy_re.compile('(http://[^\\s]*[^\\s\\.])|\n\n') > It's rather confusing to do both URL matching and paragraph splitting via the > same regular expression and code. I'd prefer you break paragraphs in a first > step and then perform link replacement in a second step. Hi Mattias, I have made changes following your feedback. Please check patch #5.
https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:61: Nit: Don't introduce a blank line here (two blank lines only between top-level definitions). https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:97: text: The string to be added.''' nit: Other docstrings in this file place a newline before the closing '''. Please do the same for consistency. https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:102: new_line_matcher = lazy_re.compile('\n\n') You can simplify this a lot by just doing text.split('\n\n'), then iterating over the result and calling AddElement and _AddTextWithLinks. https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:107: # Look for the first break line. nit: s/break line/line break/. https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:111: # Get postiotons of begin and end of new line charecters. nit: spelling https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:116: #Create a new paragraph. The old one will be automatically closed. nit: space after # https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:121: nit: remove extra blank line. https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... grit/format/policy_templates/writers/doc_writer.py:152: # Replace URLs with links and /n with <p>...</p> in the description. nit: I think "Break description into paragraphs and replace URLs with links" would be clearer.
On 2015/02/10 12:54:13, Mattias Nissler wrote: > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > File grit/format/policy_templates/writers/doc_writer.py (right): > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:61: > Nit: Don't introduce a blank line here (two blank lines only between top-level > definitions). > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:97: text: The string to be > added.''' > nit: Other docstrings in this file place a newline before the closing '''. > Please do the same for consistency. > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:102: new_line_matcher = > lazy_re.compile('\n\n') > You can simplify this a lot by just doing text.split('\n\n'), then iterating > over the result and calling AddElement and _AddTextWithLinks. > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:107: # Look for the first > break line. > nit: s/break line/line break/. > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:111: # Get postiotons of > begin and end of new line charecters. > nit: spelling > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:116: #Create a new paragraph. > The old one will be automatically closed. > nit: space after # > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:121: > nit: remove extra blank line. > > https://codereview.chromium.org/911563002/diff/80001/grit/format/policy_templ... > grit/format/policy_templates/writers/doc_writer.py:152: # Replace URLs with > links and /n with <p>...</p> in the description. > nit: I think "Break description into paragraphs and replace URLs with links" > would be clearer. Fixed. Please take a look again :)
LGTM with nits. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:97: if len(text) == 0: nit: This check is no longer needed. The loop below won't run if text is empty. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:99: # Split text into list of paragraphs nit: missing period https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:102: # Create a new paragraph node nit: period https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:104: # Insert text to the paragraph with processing the urls nit: period https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:135: nit: no reason for a blank line here AFAICT? https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:136: # Replace URLs with links and /n with <p>...</p> in the description. nit: I suggested to reword the comment to be clearer, did you forget or decide against it for some reason? If the latter, please comment why and fix s|/n|\n|.
New patchsets have been uploaded after l-g-t-m from mnissler@chromium.org
On 2015/02/10 14:09:55, Mattias Nissler wrote: > LGTM with nits. > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > File grit/format/policy_templates/writers/doc_writer.py (right): > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:97: if len(text) == 0: > nit: This check is no longer needed. The loop below won't run if text is empty. > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:99: # Split text into list of > paragraphs > nit: missing period > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:102: # Create a new paragraph > node > nit: period > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:104: # Insert text to the > paragraph with processing the urls > nit: period > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:135: > nit: no reason for a blank line here AFAICT? > > https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:136: # Replace URLs with > links and /n with <p>...</p> in the description. > nit: I suggested to reword the comment to be clearer, did you forget or decide > against it for some reason? If the latter, please comment why and fix s|/n|\n|. Hope the last attempt :)
Still LGTM, found two final nits ;) https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:102: # Insert text to the paragraph with processing the urls. nit: s/urls/URLs/ for consistency. https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:133: # Add description by paragraphs (URLs will be substituted by links) nit: missing period
New patchsets have been uploaded after l-g-t-m from mnissler@chromium.org
On 2015/02/10 14:38:01, Mattias Nissler wrote: > Still LGTM, found two final nits ;) > > https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... > File grit/format/policy_templates/writers/doc_writer.py (right): > > https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:102: # Insert text to the > paragraph with processing the urls. > nit: s/urls/URLs/ for consistency. > > https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... > grit/format/policy_templates/writers/doc_writer.py:133: # Add description by > paragraphs (URLs will be substituted by links) > nit: missing period I need to be more careful about comments :) Lets try once again :)
LGTM
peletskyi@chromium.org changed reviewers: + thakis@chromium.org
Hi, please lake a look to my changes. Best wishes, Oleksandr https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:97: if len(text) == 0: On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: This check is no longer needed. The loop below won't run if text is empty. Done. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:99: # Split text into list of paragraphs On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: missing period Done. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:102: # Create a new paragraph node On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: period Done. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:104: # Insert text to the paragraph with processing the urls On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: period Done. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:135: On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: no reason for a blank line here AFAICT? Done. https://codereview.chromium.org/911563002/diff/100001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:136: # Replace URLs with links and /n with <p>...</p> in the description. On 2015/02/10 14:09:55, Mattias Nissler wrote: > nit: I suggested to reword the comment to be clearer, did you forget or decide > against it for some reason? If the latter, please comment why and fix s|/n|\n|. I have "Break description into paragraphs and replace URLs with links." in description of the method. Don't want to have copy-paste in the comments. But I will try to change the description to make it short and clear. https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... File grit/format/policy_templates/writers/doc_writer.py (right): https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:102: # Insert text to the paragraph with processing the urls. On 2015/02/10 14:38:01, Mattias Nissler wrote: > nit: s/urls/URLs/ for consistency. Done. https://codereview.chromium.org/911563002/diff/120001/grit/format/policy_temp... grit/format/policy_templates/writers/doc_writer.py:133: # Add description by paragraphs (URLs will be substituted by links) On 2015/02/10 14:38:01, Mattias Nissler wrote: > nit: missing period Done.
tests would be nice, but if Mattias is happy I'm happy.
On 2015/02/10 16:40:56, Nico wrote: > tests would be nice, but if Mattias is happy I'm happy. So, the issue is resolved or should I write the tests?
On 2015/02/11 09:20:58, peletskyi wrote: > On 2015/02/10 16:40:56, Nico wrote: > > tests would be nice, but if Mattias is happy I'm happy. > > So, the issue is resolved or should I write the tests? There are actually quite comprehensive unit tests in doc_writer_unittest.py, let's add one there to cover paragraph breaking.
New patchsets have been uploaded after l-g-t-m from mnissler@chromium.org
On 2015/02/11 09:24:17, Mattias Nissler wrote: > On 2015/02/11 09:20:58, peletskyi wrote: > > On 2015/02/10 16:40:56, Nico wrote: > > > tests would be nice, but if Mattias is happy I'm happy. > > > > So, the issue is resolved or should I write the tests? > > There are actually quite comprehensive unit tests in doc_writer_unittest.py, > let's add one there to cover paragraph breaking. There are ~15 tests for the methods, that use _AddParagraph() inside. I've fixed that tests. From my point of view these tests are enough.
On 2015/02/12 09:25:46, peletskyi wrote: > On 2015/02/11 09:24:17, Mattias Nissler wrote: > > On 2015/02/11 09:20:58, peletskyi wrote: > > > On 2015/02/10 16:40:56, Nico wrote: > > > > tests would be nice, but if Mattias is happy I'm happy. > > > > > > So, the issue is resolved or should I write the tests? > > > > There are actually quite comprehensive unit tests in doc_writer_unittest.py, > > let's add one there to cover paragraph breaking. > > There are ~15 tests for the methods, that use _AddParagraph() inside. I've fixed > that tests. > From my point of view these tests are enough. None of the existing tests exercises behavior for more than one paragraph though. Please add a test that actually triggers paragraph-breaking.
On 2015/02/12 09:57:10, Mattias Nissler wrote: > On 2015/02/12 09:25:46, peletskyi wrote: > > On 2015/02/11 09:24:17, Mattias Nissler wrote: > > > On 2015/02/11 09:20:58, peletskyi wrote: > > > > On 2015/02/10 16:40:56, Nico wrote: > > > > > tests would be nice, but if Mattias is happy I'm happy. > > > > > > > > So, the issue is resolved or should I write the tests? > > > > > > There are actually quite comprehensive unit tests in doc_writer_unittest.py, > > > let's add one there to cover paragraph breaking. > > > > There are ~15 tests for the methods, that use _AddParagraph() inside. I've > fixed > > that tests. > > From my point of view these tests are enough. > > None of the existing tests exercises behavior for more than one paragraph > though. Please add a test that actually triggers paragraph-breaking. Done
LGTM, thanks for the test. I think it's fair to commit now given that Nico has already delegated to my magic four-letter powers.
On 2015/02/12 11:24:22, Mattias Nissler wrote: > LGTM, thanks for the test. I think it's fair to commit now given that Nico has > already delegated to my magic four-letter powers. Hi Mattias, can you commit the changes?
The CQ bit was checked by mnissler@chromium.org
The CQ bit was checked by mnissler@chromium.org
The CQ bit was unchecked by mnissler@chromium.org
On 2015/02/17 14:02:32, peletskyi wrote: > On 2015/02/12 11:24:22, Mattias Nissler wrote: > > LGTM, thanks for the test. I think it's fair to commit now given that Nico has > > already delegated to my magic four-letter powers. > > Hi Mattias, > can you commit the changes? I clicked the commit checkbox FWIW - do we know whether it works for the grit repo?
The CQ bit was checked by mnissler@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because it did not recognize the base URL. Please commit your change manually.
The CQ bit was checked by peletskyi@chromium.org
The CQ bit was unchecked by peletskyi@chromium.org
On 2015/02/17 15:00:00, I haz the power (commit-bot) wrote: > Commit queue rejected this change because it did not recognize the base URL. > Please commit your change manually. Mattias, can you commit it manually?
Message was sent while issue was closed.
Committed patchset #10 (id:180001) manually as r186 (presubmit successful).
Message was sent while issue was closed.
On 2015/02/17 15:27:42, peletskyi wrote: > On 2015/02/17 15:00:00, I haz the power (commit-bot) wrote: > > Commit queue rejected this change because it did not recognize the base URL. > > Please commit your change manually. > > Mattias, > can you commit it manually? So I've tried git cl land, which didn't work. Which made me fall back to git cl dcommit, which didn't work. Which made me fall back to an svn checkout and using gcl, which finally did work. Nico, is this sad state of committing my fault or are we really still stuck in svn/gcl land here?
Message was sent while issue was closed.
I always use `svn commit` for landing stuff in gyp and grit. I think scottmg figured out how to make git-svn work, but I didn't invest the time to get that to work – landing via svn isn't any more difficult for me. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
