Improve tidy's license validation logic #12781

UK992 · 2016-08-09T01:40:08Z

Rebased and fixed #10721, which is inactive for months.
Fixes #10716

r? @larsbergstrom or @edunham

This change is

highfive · 2016-08-09T01:40:13Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/tidy/servo_tidy_tests/test_tidy.py, python/tidy/HISTORY.rst, python/tidy/servo_tidy/tidy.py, python/tidy/servo_tidy/licenseck.py, python/tidy/setup.py, python/tidy/servo_tidy_tests/apache2_license.rs

jdm · 2016-08-09T02:53:21Z

python/tidy/servo_tidy_tests/test_tidy.py

@@ -150,3 +154,7 @@ def test_file_list(self):
 def do_tests():
    suite = unittest.TestLoader().loadTestsFromTestCase(CheckTidiness)
    return 0 if unittest.TextTestRunner(verbosity=2).run(suite).wasSuccessful() else 1
+    unittest.TextTestRunner(verbosity=2).run(suite)


This is unreachable code.

UK992 · 2016-08-09T14:22:31Z

With last commit i added grabbing actual comment block, and also added checking for blank line after shebang, so that doesn't messed up license check.

jdm · 2016-08-09T22:20:57Z

The travis failure is unrelated to these changes.

aneeshusa · 2016-08-10T02:00:41Z

python/tidy/servo_tidy/licenseck.py

@@ -7,97 +7,14 @@
 # option. This file may not be copied, modified, or distributed
 # except according to those terms.

+mpl = "This Source Code Form is subject to the terms of the Mozilla Public License, v. 2.0. If a copy of the MPL was not distributed with this file, You can obtain one at http://mozilla.org/MPL/2.0/"


Why put these onto a single line? AFAIK the indentation never changes, just the comment character at the start of each line. IMO this form is nicer:

mpl = """\ This Source Code... """,

Style Nit: Also, the name could be in uppercase, since it's a constant (even though it doesn't make any difference in python).

@aneeshusa i changed it to this type of form, and before matching with header remove all newlines.

aneeshusa · 2016-08-10T02:01:51Z

python/tidy/servo_tidy/tidy.py

+
+
+def licensed_mpl(header):
+    if licenseck.mpl in header:


We can do return licenseck.mpl in header directly.

wafflespeanut · 2016-08-10T05:47:55Z

python/tidy/servo_tidy/licenseck.py


-# These licenses are valid for use in Servo
-licenses = [
+apache = "Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or http://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your option. This file may not be copied, modified, or distributed except according to those terms."


ditto on the name

wafflespeanut · 2016-08-10T05:48:25Z

python/tidy/servo_tidy/licenseck.py

+copyright = [
+    "See the COPYRIGHT file at the top-level directory of this distribution",
+    "See http://rust-lang.org/COPYRIGHT",
+]


copyright is a built-in function. ditto on uppercase?

wafflespeanut · 2016-08-10T05:50:05Z

python/tidy/servo_tidy/tidy.py

+        yield (1, "missing blank line after shebang")
+
+    blank_lines = 0
+    MAX_BLANK_LINES = 2 if lines[0].startswith("#!") else 1


Style nit: This shouldn't be uppercase, since it's a variable.

wafflespeanut · 2016-08-10T05:53:57Z

python/tidy/servo_tidy_tests/test_tidy.py

@@ -150,3 +159,6 @@ def test_file_list(self):
 def do_tests():
    suite = unittest.TestLoader().loadTestsFromTestCase(CheckTidiness)
    return 0 if unittest.TextTestRunner(verbosity=2).run(suite).wasSuccessful() else 1
+
+if __name__ == "__main__":
+    do_tests()


Why was this change introduced?

don't know, i removed it.

wafflespeanut · 2016-08-10T05:55:33Z

python/tidy/HISTORY.rst

@@ -1,6 +1,17 @@
 Release History
 ---------------

+0.1.0 (2016-04-19)


psst... date?

wafflespeanut · 2016-08-10T05:57:13Z

python/tidy/servo_tidy/tidy.py

+
+def licensed_apache(header):
+    if licenseck.apache in header:
+        for copyright in licenseck.copyright:


ditto on copyright

wafflespeanut · 2016-08-10T05:59:31Z

python/tidy/servo_tidy/tidy.py

@@ -305,7 +339,7 @@ def check_toml(file_name, lines):
    for idx, line in enumerate(lines):
        if line.find("*") != -1:
            yield (idx + 1, "found asterisk instead of minimum version number")
-        for license in licenses_toml:
+        for license in licenseck.licenses_toml:


license is also a built-in function

UK992 · 2016-08-10T11:25:14Z

Addressed all reviews.

aneeshusa · 2016-08-10T14:54:44Z

python/tidy/servo_tidy/licenseck.py

@@ -139,3 +66,4 @@
    'name = "webrender"',
    'name = "webrender_traits"',
 ]
+# noqa: Indicate to flake8 that we do not want to check indentation here


We shouldn't need this anymore since we don't have unindented strings inside a list (the old licenses variable).

wafflespeanut · 2016-08-11T07:19:00Z

python/tidy/servo_tidy/tidy.py

@@ -17,12 +17,9 @@
 import StringIO
 import subprocess
 import sys
-from licenseck import licenses, licenses_toml, licenses_dep_toml
+import licenseck as licenses


Nit: This should be from licenseck import MPL, ... (we don't wanna prefer * even though we import all the names)

wafflespeanut · 2016-08-11T07:19:19Z

python/tidy/servo_tidy/tidy.py

-EMACS_HEADER = "/* -*- Mode:"
-VIM_HEADER = "/* vim:"
-MAX_LICENSE_LINESPAN = max(len(license.splitlines()) for license in licenses)
+COMMENTS = ["// ", "# ", " *", "/* ", "*/ "]


We won't be needing */ here, would we?

wafflespeanut · 2016-08-11T07:19:39Z

python/tidy/servo_tidy/tidy.py

+
+
+def licensed_mpl(header):
+    return licenses.MPL.replace('\n', ' ') in header


Instead of doing a string replacement for every function call, we could just have a global variable like mpl = MPL.replace('\n', ' ') or something at the top.

I added backslash to every line in licenseck.

wafflespeanut · 2016-08-11T07:19:51Z

python/tidy/servo_tidy/tidy.py

+
+
+def licensed_apache(header):
+    if licenses.APACHE.replace('\n', ' ') in header:


ditto on "globalizing"

highfive · 2016-08-12T21:23:36Z

  ▶ Unexpected subtest result in /html/browsers/browsing-the-web/history-traversal/browsing_context_name.html:
  └ PASS [expected FAIL] Retaining window.name on history traversal


        Add support colored text in Windows Console

UK992 · 2016-08-13T01:23:41Z

Last commit add support for colored text in Windows Console.
Before:

After:

wafflespeanut · 2016-08-14T07:43:23Z

Thanks again! :)

@bors-servo r+

bors-servo · 2016-08-14T07:43:24Z

📌 Commit fed0e94 has been approved by Wafflespeanut

bors-servo · 2016-08-14T08:33:51Z

⌛ Testing commit fed0e94 with merge 4b5f859...


        Auto merge of #12781 - UK992:tidycheck-rebased, r=Wafflespeanut

@larsbergstrom

Improve tidy's license validation logic Rebased and fixed #10721, which is inactive for months. Fixes #10716 r? @larsbergstrom or @edunham  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12781)

bors-servo · 2016-08-14T09:20:40Z

💔 Test failed - linux-rel

highfive · 2016-08-14T09:20:42Z

  ▶ FAIL [expected PASS] /css-transforms-1_dev/html/transform-table-007.htm
  └   → /css-transforms-1_dev/html/transform-table-007.htm a5c014b20ef1363bea6f24eda28c7efb7c45698a
/css-transforms-1_dev/html/reference/transform-blank-ref.htm fa6407b1acbbfea27e27061e7d1bdeca98e4a728
Testing a5c014b20ef1363bea6f24eda28c7efb7c45698a == fa6407b1acbbfea27e27061e7d1bdeca98e4a728

wafflespeanut · 2016-08-14T13:49:23Z

@bors-servo retry #11574

bors-servo · 2016-08-14T14:17:35Z

⌛ Testing commit fed0e94 with merge 8419f96...


        Auto merge of #12781 - UK992:tidycheck-rebased, r=Wafflespeanut

@larsbergstrom

Improve tidy's license validation logic Rebased and fixed #10721, which is inactive for months. Fixes #10716 r? @larsbergstrom or @edunham  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12781)

bors-servo · 2016-08-14T15:08:46Z

💔 Test failed - mac-rel-wpt

wafflespeanut · 2016-08-14T15:11:58Z

@bors-servo retry #12797

bors-servo · 2016-08-14T15:57:04Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, windows-dev are reusable. Rebuilding only mac-rel-wpt...

bors-servo · 2016-08-14T17:00:12Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows-dev

highfive assigned larsbergstrom Aug 9, 2016

highfive added the S-awaiting-review label Aug 9, 2016

UK992 force-pushed the UK992:tidycheck-rebased branch from 4521d53 to bd2bf06 Aug 9, 2016

jdm mentioned this pull request Aug 9, 2016

Improve tidy's license validation logic #10721

Closed

jdm reviewed Aug 9, 2016
View changes

UK992 force-pushed the UK992:tidycheck-rebased branch from bd2bf06 to d4d7948 Aug 9, 2016

jdm added S-fails-travis and removed S-fails-travis labels Aug 9, 2016

aneeshusa reviewed Aug 10, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

def licensed_mpl(header):

if licenseck.mpl in header:

This comment has been minimized.

Sign in to view

aneeshusa Aug 10, 2016
Member

We can do return licenseck.mpl in header directly.

wafflespeanut reviewed Aug 10, 2016
View changes

python/tidy/HISTORY.rst Outdated

@@ -1,6 +1,17 @@

Release History

---------------

0.1.0 (2016-04-19)

This comment has been minimized.

Sign in to view

wafflespeanut Aug 10, 2016
Member

psst... date?

wafflespeanut reviewed Aug 10, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

def licensed_apache(header):

if licenseck.apache in header:

for copyright in licenseck.copyright:

This comment has been minimized.

Sign in to view

wafflespeanut Aug 10, 2016
Member

ditto on copyright

wafflespeanut reviewed Aug 10, 2016
View changes

aneeshusa reviewed Aug 10, 2016
View changes

larsbergstrom assigned wafflespeanut and unassigned larsbergstrom and wafflespeanut Aug 10, 2016

wafflespeanut reviewed Aug 11, 2016
View changes

python/tidy/servo_tidy/tidy.py Outdated

def licensed_apache(header):

if licenses.APACHE.replace('\n', ' ') in header:

This comment has been minimized.

Sign in to view

wafflespeanut Aug 11, 2016
Member

ditto on "globalizing"

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 12, 2016

Add support colored text in Windows Console

Loading status checks…

fed0e94

highfive added S-awaiting-review and removed S-tests-failed labels Aug 13, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Aug 14, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 14, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Aug 14, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Aug 14, 2016

bors-servo merged commit fed0e94 into servo:master Aug 14, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

UK992 deleted the UK992:tidycheck-rebased branch Sep 2, 2016



		def licensed_mpl(header):
		return licenses.MPL.replace('\n', ' ') in header



		def licensed_apache(header):
		if licenses.APACHE.replace('\n', ' ') in header:

servo / servo

Join GitHub today

Improve tidy's license validation logic #12781

Improve tidy's license validation logic #12781

Conversation

UK992 commented Aug 9, 2016 • edited

highfive commented Aug 9, 2016

This comment has been minimized.

This comment has been minimized.

UK992 commented Aug 9, 2016 • edited

jdm commented Aug 9, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

UK992 commented Aug 10, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

highfive commented Aug 12, 2016

UK992 commented Aug 13, 2016 • edited

wafflespeanut commented Aug 14, 2016

bors-servo commented Aug 14, 2016

bors-servo commented Aug 14, 2016

bors-servo commented Aug 14, 2016

highfive commented Aug 14, 2016

wafflespeanut commented Aug 14, 2016

bors-servo commented Aug 14, 2016

bors-servo commented Aug 14, 2016

wafflespeanut commented Aug 14, 2016

bors-servo commented Aug 14, 2016

bors-servo commented Aug 14, 2016

UK992 commented Aug 9, 2016 •

edited

UK992 commented Aug 9, 2016 •

edited

UK992 commented Aug 13, 2016 •

edited