Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966743002:
fix comment length (Closed)
|
Created:
3 years, 5 months ago by janssonWebRTC Modified:
3 years, 5 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
Descriptionfix comment length
BUG=NONE
NOTRY=True
Review-Url: https://codereview.webrtc.org/2966743002
Cr-Commit-Position: refs/heads/master@{#18874}
Committed: https://chromium.googlesource.com/external/webrtc/+/ad515c459b817abd3c585d38181215e61ca2ccca
Patch Set 1 #
Total comments: 2
Patch Set 2 : reduce summary wording #Patch Set 3 : Merge branch 'master' into fixLine #Messages
Total messages: 28 (11 generated)
jansson@webrtc.org changed reviewers: + kjellander@webrtc.org, mbonadei@webrtc.org
Description was changed from ========== fix comment length BUG=NONE ========== to ========== fix comment length BUG=NONE NOTRY=TRUE ==========
Description was changed from ========== fix comment length BUG=NONE NOTRY=TRUE ========== to ========== fix comment length BUG=NONE NOTRY=True ==========
https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... File webrtc/rtc_tools/video_analysis.py (right): https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py script from src/webrtc/rtc_tools using the file I think this violates coding standards. It should stay on one line and if there are too many things to explain then it should go in the section before "Args:". Something like: """Runs the compare_video.py script from src/webrtc/rtc_tools using the path. ... Args: """
https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... File webrtc/rtc_tools/video_analysis.py (right): https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py script from src/webrtc/rtc_tools using the file On 2017/06/30 11:58:15, mbonadei wrote: > I think this violates coding standards. It should stay on one line > and if there are too many things to explain then it should go in the > section before "Args:". > > Something like: > > """Runs the compare_video.py script from src/webrtc/rtc_tools using the path. > > ... > > Args: > """ Yepp, first line should fit on one line. Anything else should be in the next paragraph. Nittypicky, yes.
On 2017/06/30 12:05:17, kjellander_webrtc wrote: > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > File webrtc/rtc_tools/video_analysis.py (right): > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py script from > src/webrtc/rtc_tools using the file > On 2017/06/30 11:58:15, mbonadei wrote: > > I think this violates coding standards. It should stay on one line > > and if there are too many things to explain then it should go in the > > section before "Args:". > > > > Something like: > > > > """Runs the compare_video.py script from src/webrtc/rtc_tools using the path. > > > > ... > > > > Args: > > """ > > Yepp, first line should fit on one line. > Anything else should be in the next paragraph. Nittypicky, yes. Argh, presubmit should catch this but I guess it doesn't run for .py due to: Running presubmit upload checks ... Ignoring /src/webrtc/src/webrtc/rtc_tools/video_analysis.py; not a valid file name (cc, h, cpp, cu, cuh)
Done
On 2017/06/30 12:08:24, janssonWebRTC wrote: > On 2017/06/30 12:05:17, kjellander_webrtc wrote: > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > File webrtc/rtc_tools/video_analysis.py (right): > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py script > from > > src/webrtc/rtc_tools using the file > > On 2017/06/30 11:58:15, mbonadei wrote: > > > I think this violates coding standards. It should stay on one line > > > and if there are too many things to explain then it should go in the > > > section before "Args:". > > > > > > Something like: > > > > > > """Runs the compare_video.py script from src/webrtc/rtc_tools using the > path. > > > > > > ... > > > > > > Args: > > > """ > > > > Yepp, first line should fit on one line. > > Anything else should be in the next paragraph. Nittypicky, yes. > > Argh, presubmit should catch this but I guess it doesn't run for .py due to: > Running presubmit upload checks ... > Ignoring /src/webrtc/src/webrtc/rtc_tools/video_analysis.py; not a valid file > name (cc, h, cpp, cu, cuh) It runs for all python files but we don't have all rules enabled. I did a large effort for that but there's still many rules left to enable.
On 2017/06/30 13:13:49, kjellander_webrtc wrote: > On 2017/06/30 12:08:24, janssonWebRTC wrote: > > On 2017/06/30 12:05:17, kjellander_webrtc wrote: > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > File webrtc/rtc_tools/video_analysis.py (right): > > > > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py script > > from > > > src/webrtc/rtc_tools using the file > > > On 2017/06/30 11:58:15, mbonadei wrote: > > > > I think this violates coding standards. It should stay on one line > > > > and if there are too many things to explain then it should go in the > > > > section before "Args:". > > > > > > > > Something like: > > > > > > > > """Runs the compare_video.py script from src/webrtc/rtc_tools using the > > path. > > > > > > > > ... > > > > > > > > Args: > > > > """ > > > > > > Yepp, first line should fit on one line. > > > Anything else should be in the next paragraph. Nittypicky, yes. > > > > Argh, presubmit should catch this but I guess it doesn't run for .py due to: > > Running presubmit upload checks ... > > Ignoring /src/webrtc/src/webrtc/rtc_tools/video_analysis.py; not a valid file > > name (cc, h, cpp, cu, cuh) > > It runs for all python files but we don't have all rules enabled. I did a large > effort for that but there's still many rules left to enable. Why does it say that it's ignoring the file then?
On 2017/06/30 13:26:18, janssonWebRTC wrote: > On 2017/06/30 13:13:49, kjellander_webrtc wrote: > > On 2017/06/30 12:08:24, janssonWebRTC wrote: > > > On 2017/06/30 12:05:17, kjellander_webrtc wrote: > > > > > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > > File webrtc/rtc_tools/video_analysis.py (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > > webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py > script > > > from > > > > src/webrtc/rtc_tools using the file > > > > On 2017/06/30 11:58:15, mbonadei wrote: > > > > > I think this violates coding standards. It should stay on one line > > > > > and if there are too many things to explain then it should go in the > > > > > section before "Args:". > > > > > > > > > > Something like: > > > > > > > > > > """Runs the compare_video.py script from src/webrtc/rtc_tools using the > > > path. > > > > > > > > > > ... > > > > > > > > > > Args: > > > > > """ > > > > > > > > Yepp, first line should fit on one line. > > > > Anything else should be in the next paragraph. Nittypicky, yes. > > > > > > Argh, presubmit should catch this but I guess it doesn't run for .py due to: > > > Running presubmit upload checks ... > > > Ignoring /src/webrtc/src/webrtc/rtc_tools/video_analysis.py; not a valid > file > > > name (cc, h, cpp, cu, cuh) > > > > It runs for all python files but we don't have all rules enabled. I did a > large > > effort for that but there's still many rules left to enable. > > Why does it say that it's ignoring the file then? It's cpplint that is ignoring the file, not PyLint. It's a poor message, I agree. lgtm
The CQ bit was checked by jansson@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/06/30 13:30:57, kjellander_webrtc wrote: > On 2017/06/30 13:26:18, janssonWebRTC wrote: > > On 2017/06/30 13:13:49, kjellander_webrtc wrote: > > > On 2017/06/30 12:08:24, janssonWebRTC wrote: > > > > On 2017/06/30 12:05:17, kjellander_webrtc wrote: > > > > > > > > > > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > > > File webrtc/rtc_tools/video_analysis.py (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2966743002/diff/1/webrtc/rtc_tools/video_analys... > > > > > webrtc/rtc_tools/video_analysis.py:411: """Runs the compare_video.py > > script > > > > from > > > > > src/webrtc/rtc_tools using the file > > > > > On 2017/06/30 11:58:15, mbonadei wrote: > > > > > > I think this violates coding standards. It should stay on one line > > > > > > and if there are too many things to explain then it should go in the > > > > > > section before "Args:". > > > > > > > > > > > > Something like: > > > > > > > > > > > > """Runs the compare_video.py script from src/webrtc/rtc_tools using > the > > > > path. > > > > > > > > > > > > ... > > > > > > > > > > > > Args: > > > > > > """ > > > > > > > > > > Yepp, first line should fit on one line. > > > > > Anything else should be in the next paragraph. Nittypicky, yes. > > > > > > > > Argh, presubmit should catch this but I guess it doesn't run for .py due > to: > > > > Running presubmit upload checks ... > > > > Ignoring /src/webrtc/src/webrtc/rtc_tools/video_analysis.py; not a valid > > file > > > > name (cc, h, cpp, cu, cuh) > > > > > > It runs for all python files but we don't have all rules enabled. I did a > > large > > > effort for that but there's still many rules left to enable. > > > > Why does it say that it's ignoring the file then? > > It's cpplint that is ignoring the file, not PyLint. It's a poor message, I > agree. > lgtm I see, yeah it should prefix the linter name before printing messages.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18776)
The CQ bit was checked by jansson@webrtc.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18778)
On 2017/06/30 13:56:42, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > presubmit on master.tryserver.webrtc (JOB_FAILED, > http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18778) I don't understand why it failed again. Have you rebased?
The CQ bit was checked by jansson@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2966743002/#ps40001 (title: "Merge branch 'master' into fixLine")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499067886155710,
"parent_rev": "a80c16a67ca354ef924393b7fc6fba1d1daef453", "commit_rev":
"ad515c459b817abd3c585d38181215e61ca2ccca"}
Message was sent while issue was closed.
Description was changed from ========== fix comment length BUG=NONE NOTRY=True ========== to ========== fix comment length BUG=NONE NOTRY=True Review-Url: https://codereview.webrtc.org/2966743002 Cr-Commit-Position: refs/heads/master@{#18874} Committed: https://chromium.googlesource.com/external/webrtc/+/ad515c459b817abd3c585d381... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/external/webrtc/+/ad515c459b817abd3c585d381... |
