Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2210633002:
Modify java_cpp_enum.py to preserve comments (Closed)
|
Created:
4 years, 4 months ago by xunjieli Modified:
4 years, 4 months ago Reviewers:
agrieve CC:
chromium-reviews, jbudorick+watch_chromium.org, mikecase+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionModify java_cpp_enum.py to preserve comments
This CL modifies java_cpp_enum.py to preserve comments on the
enums in the generated Java file.
The comments are useful for developers who usually read the
JavaDoc and not the C++ files.
BUG=634092
Committed: https://crrev.com/ea986397d77dc45b5b272ad68d00cc6063ee6987
Cr-Commit-Position: refs/heads/master@{#410047}
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 3
Patch Set 3 : break up long line #
Messages
Total messages: 33 (24 generated)
Patchset #1 (id:1) has been deleted
xunjieli@chromium.org changed reviewers: + agrieve@chromium.org
PTAL?
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifis java_cpp_enum.py to preserve comments on the enums in the generated Java file. ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifis java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ==========
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifis java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifis java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ==========
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifis java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ==========
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. BUG=634092 ==========
Nice change! lgtm /w nits. https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... File build/android/gyp/java_cpp_enum.py (right): https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:182: enum_comment = HeaderParser.single_line_comment_re.match(line).groups()[0] nit: Avoid calling match() twice using a variable. https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:184: self._current_comments.append(' ') nit: Could you just use ' '.join() later on rather than adding in spaces? https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:311: initial_indent = enum_comments_indent, nit: no spaces around = for arguments. initial_indent=enum_comments_indent,
Thanks for the review! https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... File build/android/gyp/java_cpp_enum.py (right): https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:182: enum_comment = HeaderParser.single_line_comment_re.match(line).groups()[0] On 2016/08/03 23:09:53, agrieve wrote: > nit: Avoid calling match() twice using a variable. Done. https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:184: self._current_comments.append(' ') On 2016/08/03 23:09:53, agrieve wrote: > nit: Could you just use ' '.join() later on rather than adding in spaces? Done. Ah, you are right! I didn't thought about that. https://codereview.chromium.org/2210633002/diff/20001/build/android/gyp/java_... build/android/gyp/java_cpp_enum.py:311: initial_indent = enum_comments_indent, On 2016/08/03 23:09:53, agrieve wrote: > nit: no spaces around = for arguments. initial_indent=enum_comments_indent, Done.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... File build/android/gyp/java_cpp_enum_tests.py (right): https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... build/android/gyp/java_cpp_enum_tests.py:61: * This is a multiple line comment that is really long. This is a multiple line comment that is Is there a way to bypass the "Found lines longer than 80 characters" error from Presubmit? Is adding "#pylint: disable" the way to go? or is there a way to break up this string? Thanks!
https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... File build/android/gyp/java_cpp_enum_tests.py (right): https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... build/android/gyp/java_cpp_enum_tests.py:61: * This is a multiple line comment that is really long. This is a multiple line comment that is On 2016/08/04 14:19:31, xunjieli wrote: > Is there a way to bypass the "Found lines longer than 80 characters" error from > Presubmit? > > Is adding "#pylint: disable" the way to go? or is there a way to break up this > string? > > Thanks! Another option would be to use %s%s or str.format() to put in longer line via substitution. I'm fine if you can get pylint:disable to work as well though.
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by xunjieli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... File build/android/gyp/java_cpp_enum_tests.py (right): https://codereview.chromium.org/2210633002/diff/40001/build/android/gyp/java_... build/android/gyp/java_cpp_enum_tests.py:61: * This is a multiple line comment that is really long. This is a multiple line comment that is On 2016/08/04 16:39:39, agrieve wrote: > On 2016/08/04 14:19:31, xunjieli wrote: > > Is there a way to bypass the "Found lines longer than 80 characters" error > from > > Presubmit? > > > > Is adding "#pylint: disable" the way to go? or is there a way to break up this > > string? > > > > Thanks! > > Another option would be to use %s%s or str.format() to put in longer line via > substitution. > > I'm fine if you can get pylint:disable to work as well though. Done. I couldn't get pylint:disable to work for the string block. It works on a single line. I used str.format() approach.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from agrieve@chromium.org Link to the patchset: https://codereview.chromium.org/2210633002/#ps80001 (title: "break up long line")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. BUG=634092 ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. BUG=634092 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. BUG=634092 ========== to ========== Modify java_cpp_enum.py to preserve comments This CL modifies java_cpp_enum.py to preserve comments on the enums in the generated Java file. The comments are useful for developers who usually read the JavaDoc and not the C++ files. BUG=634092 Committed: https://crrev.com/ea986397d77dc45b5b272ad68d00cc6063ee6987 Cr-Commit-Position: refs/heads/master@{#410047} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/ea986397d77dc45b5b272ad68d00cc6063ee6987 Cr-Commit-Position: refs/heads/master@{#410047} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
