Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2394753002:
Suppress the AlertDialog presubmit warning when not useful (Closed)
DescriptionSuppress the AlertDialog presubmit warning when not useful
This warning is only useful in chrome/, blimp/, and remoting/, which can
depend on the support library.
On the other hand, content layer cannot depend on the support library.
Therefore, all the directories containing Java packages that could be
imported by content layer are suppressed.
BUG=640248
Committed: https://crrev.com/9fc653a91c70e0a2357e83fdc16f6cc9c80fb23f
Cr-Commit-Position: refs/heads/master@{#423922}
Patch Set 1 #
Total comments: 2
Patch Set 2 : add comments #Patch Set 3 : list all directories #Messages
Total messages: 21 (8 generated)
wychen@chromium.org changed reviewers: + stip@chromium.org, tedchoc@chromium.org
PTAL
Description was changed from ========== Suppress the AlertDialog presubmit warning in src/content Content layer cannot depend on the support library. BUG=640248 ========== to ========== Suppress the AlertDialog presubmit warning in content/ Content layer cannot depend on the support library. BUG=640248 ==========
might want to leave the comment in there, replacing it with this justification lgtm
lgtm https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... File tools/android/checkstyle/suppressions.xml (right): https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... tools/android/checkstyle/suppressions.xml:6: <suppress id="AlertDialogCheck" files="src/content/"/> really, we want to suppress this for every directory but chrome...don't know if that is possible though.
On 2016/10/05 16:08:33, stip wrote: > might want to leave the comment in there, replacing it with this justification > > lgtm Comment added.
wychen@chromium.org changed reviewers: + jbudorick@chromium.org
jbudorick@, can you take a look? Thanks!
lgtm
https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... File tools/android/checkstyle/suppressions.xml (right): https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... tools/android/checkstyle/suppressions.xml:6: <suppress id="AlertDialogCheck" files="src/content/"/> On 2016/10/05 16:10:12, Ted C wrote: > really, we want to suppress this for every directory but chrome...don't know if > that is possible though. What about blimp/ and remoting/? They already have android.support.v7.app.AlertDialog usage. I think it makes sense to add these to our whitelist as well. This suppression list only works by using a regex to match file names, so the best we can do here is to enumerate all the directory names under src/ except for the whitelisted ones. It's not ideal, but since it's pretty rare to add top-level directories under src/, we don't have to worry about updating this often.
On 2016/10/06 23:12:28, wychen wrote: > https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... > File tools/android/checkstyle/suppressions.xml (right): > > https://codereview.chromium.org/2394753002/diff/1/tools/android/checkstyle/su... > tools/android/checkstyle/suppressions.xml:6: <suppress id="AlertDialogCheck" > files="src/content/"/> > On 2016/10/05 16:10:12, Ted C wrote: > > really, we want to suppress this for every directory but chrome...don't know > if > > that is possible though. > > What about blimp/ and remoting/? They already have > android.support.v7.app.AlertDialog usage. I think it makes sense to add these to > our whitelist as well. > > This suppression list only works by using a regex to match file names, so the > best we can do here is to enumerate all the directory names under src/ except > for the whitelisted ones. It's not ideal, but since it's pretty rare to add > top-level directories under src/, we don't have to worry about updating this > often. Definitely! Anything directory that supports the support library should use this checkstyle rule. My chrome/ comment was a drastic oversimplification.
Description was changed from ========== Suppress the AlertDialog presubmit warning in content/ Content layer cannot depend on the support library. BUG=640248 ========== to ========== Suppress the AlertDialog presubmit warning when not useful This warning is only useful in chrome/, blimp/, and remoting/, which can depend on the support library. On the other hand, content layer cannot depend on the support library. Therefore, all the directories containing Java packages that could be imported by content layer are suppressed. BUG=640248 ==========
On 2016/10/06 23:50:56, Ted C wrote: > Definitely! Anything directory that supports the support library should use > this checkstyle rule. My chrome/ comment was a drastic oversimplification. Updated the filter to enumerate directories, and also updated the CL description. PTAL at the description to see if it's accurate.
On 2016/10/07 01:20:34, wychen wrote: > On 2016/10/06 23:50:56, Ted C wrote: > > Definitely! Anything directory that supports the support library should use > > this checkstyle rule. My chrome/ comment was a drastic oversimplification. > > Updated the filter to enumerate directories, and also updated the CL > description. > PTAL at the description to see if it's accurate. lgtm thanks!
The CQ bit was checked by wychen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stip@chromium.org, jbudorick@chromium.org Link to the patchset: https://codereview.chromium.org/2394753002/#ps40001 (title: "list all directories")
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 ========== Suppress the AlertDialog presubmit warning when not useful This warning is only useful in chrome/, blimp/, and remoting/, which can depend on the support library. On the other hand, content layer cannot depend on the support library. Therefore, all the directories containing Java packages that could be imported by content layer are suppressed. BUG=640248 ========== to ========== Suppress the AlertDialog presubmit warning when not useful This warning is only useful in chrome/, blimp/, and remoting/, which can depend on the support library. On the other hand, content layer cannot depend on the support library. Therefore, all the directories containing Java packages that could be imported by content layer are suppressed. BUG=640248 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Suppress the AlertDialog presubmit warning when not useful This warning is only useful in chrome/, blimp/, and remoting/, which can depend on the support library. On the other hand, content layer cannot depend on the support library. Therefore, all the directories containing Java packages that could be imported by content layer are suppressed. BUG=640248 ========== to ========== Suppress the AlertDialog presubmit warning when not useful This warning is only useful in chrome/, blimp/, and remoting/, which can depend on the support library. On the other hand, content layer cannot depend on the support library. Therefore, all the directories containing Java packages that could be imported by content layer are suppressed. BUG=640248 Committed: https://crrev.com/9fc653a91c70e0a2357e83fdc16f6cc9c80fb23f Cr-Commit-Position: refs/heads/master@{#423922} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9fc653a91c70e0a2357e83fdc16f6cc9c80fb23f Cr-Commit-Position: refs/heads/master@{#423922} |
