Have tidy ensure that the there are no extra files in the webidls folder #13427 #13447

gterzian · 2016-09-27T08:07:04Z

Have tidy ensure that the there are no extra files in the webidls folder #13427

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #13427 (github issue number if applicable).

There are tests for these changes

This change is

highfive · 2016-09-27T08:07:10Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @Manishearth (or someone else) soon.

highfive · 2016-09-27T08:07:10Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/tidy/README.rst

gterzian · 2016-09-27T08:10:53Z

WIP

wafflespeanut · 2016-09-27T08:33:46Z

Ping me once you've made the changes :)

Manishearth · 2016-09-27T08:39:34Z

r? @wafflespeanut

gterzian · 2016-09-27T10:06:17Z

@wafflespeanut @Manishearth good for review 👍

gterzian · 2016-09-27T11:30:04Z

@wafflespeanut @Manishearth any ideas on that build failure? It's a little hard to make out what the failure was...

Manishearth · 2016-09-27T11:34:52Z

Ignore it, servo's travis is broken right now. The first travis build runs tidy and that's enough for now.

Ms2ger · 2016-09-27T13:30:55Z

Should DIRECTORIES_FOR_FILETYPES live in the config file?

wafflespeanut · 2016-09-28T03:19:14Z

python/tidy/README.rst

+
+```
+./mach test-tidy --no-progress --self-test
+```


Have a separate commit for the changes to README

wafflespeanut · 2016-09-28T03:19:14Z

python/tidy/servo_tidy/tidy.py

+DIRECTORIES_FOR_FILETYPES = {
+    "components/script/dom/webidls": [".webidl"]
+}
+


Like @Ms2ger said, this should live in the config file (servo-tidy.toml in the root dir), and the dict should be obtained from the config variable.

wafflespeanut · 2016-09-28T03:19:14Z

python/tidy/servo_tidy/tidy.py

@@ -730,6 +734,19 @@ def parse_config(content):
            config[pref] = user_configs[pref]


+def check_directory_files(directories_for_files):
+    for directory, file_extensions in directories_for_files.items():
+        files = get_file_list(directory, only_changed_files=False, exclude_dirs=[])


only_changed_files and exclude_dirs should be brought in as an argument, same as how we do for other functions.

wafflespeanut · 2016-09-28T03:19:14Z

python/tidy/servo_tidy/tidy.py

+        files = get_file_list(directory, only_changed_files=False, exclude_dirs=[])
+        for filename in files:
+            expected = [True for possible in file_extensions if filename.endswith(possible)]
+            if not expected:


This could be changed to,

if not any(filename.endswith(ext) for ext in file_extensions):

@Ms2ger @wafflespeanut thank you both for your review, I have implemented the suggested changes, please take another look...

wafflespeanut · 2016-09-28T11:48:49Z

servo-tidy.toml

@@ -46,3 +46,8 @@ directories = [
  "./target",
  "./ports/cef",
 ]
+
+# Directories that are checked for correct file types
+[check_dirs_for_exts]


Maybe this should have a different name? (like, force_ext?)

good one, and maybe check_ext is really the best description of what it does?

Yeah, check_ext is better :)

wafflespeanut · 2016-09-28T11:57:48Z

python/tidy/servo_tidy/tidy.py

+                    'wrong': filename.split('/')[-1],
+                    'dir': directory
+                }
+                yield (filename, 1, "found file with unexpected extension: {wrong} in {dir}".format(**error_details))


This could be more generic like,

"... unexpected extension found for {name}. We only expect {ext} in {dir_name}".format(name=os.path.basename(filename), ext=', '.join(file_extensions), dir_name=os.path.dirname(filename)"

Maybe put those stuff in that dictionary before formatting the error?

good one 👍

wafflespeanut · 2016-09-28T11:58:59Z

python/tidy/servo_tidy/tidy.py

@@ -723,13 +725,32 @@ def parse_config(content):
    config['ignore']['directories'] = map(lambda path: os.path.join(*path.split('/')),
                                          config['ignore']['directories'])

+    # Add dict of dir, list of expected ext to config
+    dirs_for_ext = config_file.get("check_dirs_for_exts", {})


Since we're using get with defaults, maybe we don't want it in the default config?

do you mean removing https://github.com/gterzian/servo/blob/efb85e9e0c70e7e85b9d74ba54782c1e98c139cb/python/tidy/servo_tidy/tidy.py#L39 ?

Yes, since we're getting {} by default anyway :)

By the way, the get with default {} is when accessing the parsed toml file (see this line),
as opposed to the python config object that is declared here

If I remove the check_ext key from the default python config object at the top of the page, then at some point in parse_config I will have to do a config['check_ext'] = {}.

Perhaps actually 'declaring' this key at the top of the page, is more explicit than adding it later elsewhere in the code? Someone later might wonder where that key is coming from, thinking the object on top of the page is the proper 'template' for the config object...

I also have another small problem :)

Currently I pass the directories to check as an argument to the check_directory_files function, inside the main scan function, by actually grabbing from the config(see). This makes testing that function very easy, as in the test I just pass another list of directories (see)

The funny thing is that, at this point in the scan function, the config hasn't been parsed yet. The parsing only occurs later, during the first iteration of the generators :)

So if I remove the default key on top, I get a KeyError at that point.

If I keep the default key, it works, because the initially empty dict is later mutated during parsing, so by the time the exectution gets to here we have the value we expect.

So the question is, should I move the access to config to inside check_directory_files? If I do, it makes for harder testing of that function, since I would have to mock the config object. Actually, since the object is defined in the same module as the function, and not imported, I'm not sure it could be mocked in testing... (well I guess I could mock toml.loads)

What do you think?

OK I have found a solution, I think, I'll push the code and we'll be able to discuss in more concrete way :)

Ok, ready for review 👍

I removed the default key in the config, and I'm mocking the config in the tests... Other suggested changes have been implemented as well...

gterzian · 2016-10-02T06:42:26Z

@wafflespeanut @jdm Ok, I think this was due to the fact that the order of filenames returned by os.listdir is not necessarily the same, so, now applying sorted to the names first. That should fix it. (not just to make the test pass, I guess it makes sense to sort the name for display in potential errors anyway...)

wafflespeanut · 2016-10-02T08:01:57Z

I'm surprised, because Travis didn't show a failure on that earlier.

@bors-servo r+

bors-servo · 2016-10-02T08:01:57Z

📌 Commit 62d9882 has been approved by Wafflespeanut

bors-servo · 2016-10-02T08:02:00Z

⌛ Testing commit 62d9882 with merge f9914d5...


        Auto merge of #13447 - gterzian:check_webidls_folder_files, r=Waffles…

…peanut Have tidy ensure that the there are no extra files in the webidls folder #13427  Have tidy ensure that the there are no extra files in the webidls folder #13427 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13427 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13447)

bors-servo · 2016-10-02T08:41:17Z

💔 Test failed - mac-rel-wpt2

wafflespeanut · 2016-10-02T08:43:09Z

@bors-servo retry #13535

bors-servo · 2016-10-02T09:49:38Z

⌛ Testing commit 62d9882 with merge 277e2eb...


        Auto merge of #13447 - gterzian:check_webidls_folder_files, r=Waffles…

…peanut Have tidy ensure that the there are no extra files in the webidls folder #13427  Have tidy ensure that the there are no extra files in the webidls folder #13427 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13427 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13447)

bors-servo · 2016-10-02T10:36:44Z

💔 Test failed - mac-rel-wpt1

highfive · 2016-10-02T10:36:46Z

  ▶ Unexpected subtest result in /html/webappapis/scripting/event-loops/microtask_after_raf.html:
  └ PASS [expected FAIL] Microtask execute immediately after script

jdm · 2016-10-02T15:33:20Z

@bors-servo: retry

#13501

bors-servo · 2016-10-02T17:00:37Z

⌛ Testing commit 62d9882 with merge 3a5fd8b...


        Auto merge of #13447 - gterzian:check_webidls_folder_files, r=Waffles…

…peanut Have tidy ensure that the there are no extra files in the webidls folder #13427  Have tidy ensure that the there are no extra files in the webidls folder #13427 ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #13427 (github issue number if applicable).  - [x] There are tests for these changes   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13447)

bors-servo · 2016-10-02T18:02:12Z

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

highfive assigned Manishearth Sep 27, 2016

highfive added the S-awaiting-review label Sep 27, 2016

highfive assigned wafflespeanut and unassigned Manishearth Sep 27, 2016

gterzian force-pushed the gterzian:check_webidls_folder_files branch 3 times, most recently from cb256ab to e6f7afe Sep 27, 2016

gterzian force-pushed the gterzian:check_webidls_folder_files branch from e6f7afe to 91262af Sep 27, 2016

gterzian force-pushed the gterzian:check_webidls_folder_files branch from 91262af to a5fff06 Sep 27, 2016

wafflespeanut requested changes Sep 28, 2016

View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Sep 28, 2016

gterzian force-pushed the gterzian:check_webidls_folder_files branch from a5fff06 to 07955ba Sep 28, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 28, 2016

gterzian force-pushed the gterzian:check_webidls_folder_files branch 2 times, most recently from 054b5e9 to efb85e9 Sep 28, 2016

wafflespeanut reviewed Sep 28, 2016

View changes

wafflespeanut added the S-needs-code-changes label Sep 29, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Oct 2, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 2, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 2, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 2, 2016

highfive removed the S-awaiting-merge label Oct 2, 2016

highfive added the S-tests-failed label Oct 2, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 2, 2016

bors-servo merged commit 62d9882 into servo:master Oct 2, 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

highfive removed the S-awaiting-merge label Oct 2, 2016

gterzian deleted the gterzian:check_webidls_folder_files branch Jan 7, 2020

servo / servo

Join GitHub today

Have tidy ensure that the there are no extra files in the webidls folder #13427 #13447

Have tidy ensure that the there are no extra files in the webidls folder #13427 #13447

Conversation

gterzian commented Sep 27, 2016 • edited by wafflespeanut

highfive commented Sep 27, 2016

highfive commented Sep 27, 2016

gterzian commented Sep 27, 2016

wafflespeanut commented Sep 27, 2016

Manishearth commented Sep 27, 2016

gterzian commented Sep 27, 2016

gterzian commented Sep 27, 2016

Manishearth commented Sep 27, 2016

Ms2ger commented Sep 27, 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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian Sep 29, 2016 • edited

This comment has been minimized.

gterzian Sep 29, 2016 • edited

This comment has been minimized.

This comment has been minimized.

gterzian commented Oct 2, 2016 • edited

wafflespeanut commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

wafflespeanut commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

highfive commented Oct 2, 2016

jdm commented Oct 2, 2016

bors-servo commented Oct 2, 2016

bors-servo commented Oct 2, 2016

gterzian commented Sep 27, 2016 •

edited by wafflespeanut

gterzian Sep 29, 2016 •

edited

gterzian Sep 29, 2016 •

edited

gterzian commented Oct 2, 2016 •

edited