Tidy config file #12877

UK992 · 2016-08-15T14:33:55Z

This is wip workaround for #10841
Adds servo-tidy.toml with configs and ignored dirs, files and packages.
This will allow to set custom configuration per repo.
It's an example how could config file looks like.
I want opinion on that, if this is right approaches and how to improve it.

cc @edunham

This change is

highfive · 2016-08-15T14:33:59Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/tidy/servo_tidy/tidy.py

edunham · 2016-08-15T18:49:47Z

python/tidy/servo_tidy/tidy.py

+import toml
+
+# Load configs from servo-tidy.toml if config file exists
+if os.path.exists("./servo-tidy.toml"):


Please catch the case in which the config file does not exist, as well. Printing "./servo-tidy.toml config file is required but was not found" and exiting would be fine.

A user who naively deletes the config file will get a confusing pile of errors later when the code tries to read from a var like exclude that only exists if the config file exists and was read successfully, so let's save them from that.

Also, why have you split the config-file-reading logic between the top level here and the set_configs function down at L86?

set_config was meant for override default configs. Top-level logic was to set constants for ignored files/dirs, But now its added to config under ignore.

edunham · 2016-08-15T18:57:46Z

python/tidy/servo_tidy/tidy.py

@@ -104,6 +76,23 @@
    " accessible to\n// web pages."
 ]

+# Default configs
+config = {


Why do you have a default config dict, but no default for exclude or ignored_dirs? (up on L25, see other line note)

edunham · 2016-08-15T19:11:41Z

Thank you for the PR! The config file looks great to me.

However, it looks like you're taking two separate approaches to how the config file should be treated, and I'd personally find the code much easier to read and debug if you standardized on one or the other. The approaches that I'm seeing you take are:

for exclude and ignored_dirs, you're only setting them if servo_tidy.toml exists. The config file is mandatory, for if it's absent, code like L290 of tidy.py will blow up on the undefined variables.
For config, you're setting a default value (L80 of tidy.py) and gracefully failing back to the default if nothing is set in the config file or if the config file doesn't even exist. This makes the config file optional and places more importance on the hardcoded defaults.

Either approach would be ok -- (1) would probably be easier to implement, requiring fewer lines of code and less reasoning about defaults, whereas (2) would make tidy behave more gracefully (though perhaps in a silently wrong manner, depending on the project) if it was used without a config file or with a malformed config.

For tidy's current usage, I think the config file is mandatory. Regardless of whether you implement default values for missing configs, the absence of a config file should cause tidy to exit with a helpful error message.

UK992 · 2016-08-15T19:51:16Z

Now its unified to config defaults, more simplified.

edunham · 2016-08-16T17:45:23Z

Looks great to me! Thanks for making those changes.

@wafflespeanut, want to take a quick look too, since highfive picked you for this one?

wafflespeanut · 2016-08-16T19:24:07Z

Of course! This is in my queue. I'll look into it :)

UK992 · 2016-08-16T20:17:28Z

Last commit adds checking for wrong keys or tables in tidy config file. This will prevent adding wrong keys/tables.

bors-servo · 2016-08-17T20:11:21Z

☔ The latest upstream changes (presumably #11756) made this pull request unmergeable. Please resolve the merge conflicts.

wafflespeanut · 2016-08-18T13:36:32Z

Sorry I let this bitrot. I'll look into this tonight 😄

wafflespeanut · 2016-08-18T18:47:09Z

python/tidy/servo_tidy/tidy.py

@@ -706,11 +737,16 @@ def collect_errors_for_files(files_to_check, checking_functions, line_checking_f
            continue
        with open(filename, "r") as f:
            contents = f.read()
+            lines = contents.splitlines(True)
+            if filename.endswith(os.path.join(os.sep, "servo-tidy.toml")):


This may not be the correct place to check the config file, since that's probably the first thing that we should be doing. Let's move check_config_file to scan, have a separate variable binding config_errors for this thing, and chain with the other iterators in such a way that this is done first.

wafflespeanut · 2016-08-18T18:52:42Z

python/tidy/servo_tidy/tidy.py

+}
+
+# Load configs from servo-tidy.toml
+if os.path.exists("./servo-tidy.toml"):


Looks like we're using "servo-tidy.toml" throughout the code - maybe put that as a constant at the top (say, CONFIG_FILE_PATH?)

wafflespeanut · 2016-08-18T18:57:39Z

python/tidy/servo_tidy/tidy.py

@@ -694,6 +695,36 @@ def check_spec(file_name, lines):
                brace_count -= 1


+def check_config_file(lines):
+    current_table = ""
+    for idx, line in enumerate(lines):


Since we've planned to prioritize this check, instead of relying on collect_errors_for_files, we should initially check whether the config file exists, get the contents, and begin tidying.

wafflespeanut · 2016-08-20T20:32:09Z

Can you please squash the commits appropriately?

UK992 · 2016-08-20T20:40:00Z

done.


        Tidy config file


        Check for errors in tidy config file

wafflespeanut · 2016-08-21T03:18:29Z

I still have a few nits, but I don't wanna be nitpicky about those. I'll take care of it later. Thanks for doing this! :)

@bors-servo r+

bors-servo · 2016-08-21T03:18:31Z

📌 Commit 74dba5c has been approved by Wafflespeanut

bors-servo · 2016-08-21T03:27:06Z

⌛ Testing commit 74dba5c with merge 1c9650c...


        Auto merge of #12877 - UK992:tidy-toml, r=Wafflespeanut

@edunham

Tidy config file This is wip workaround for #10841 Adds ``servo-tidy.toml`` with configs and ignored dirs, files and packages. This will allow to set custom configuration per repo. It's an example how could config file looks like. I want opinion on that, if this is right approaches and how to improve it. cc @edunham  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12877)

bors-servo · 2016-08-21T04:13:03Z

💔 Test failed - linux-rel

highfive · 2016-08-21T04:13:07Z

  ▶ 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-21T04:17:17Z

@bors-servo retry #11574

bors-servo · 2016-08-21T04:59:27Z

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

bors-servo · 2016-08-21T05:47:08Z

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

highfive assigned pcwalton Aug 15, 2016

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

UK992 force-pushed the UK992:tidy-toml branch from fdafa75 to 82dc399 Aug 15, 2016

edunham reviewed Aug 15, 2016
View changes

edunham mentioned this pull request Aug 16, 2016

RFC: Tidy config file #10841

Closed

UK992 force-pushed the UK992:tidy-toml branch 4 times, most recently from 28a59fe to 6add322 Aug 17, 2016

highfive added the S-needs-rebase label Aug 17, 2016

UK992 force-pushed the UK992:tidy-toml branch from 6add322 to 070f6ef Aug 17, 2016

wafflespeanut reviewed Aug 18, 2016
View changes

wafflespeanut added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Aug 18, 2016

wafflespeanut assigned wafflespeanut and unassigned pcwalton Aug 18, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 18, 2016

wafflespeanut added S-needs-code-changes and removed S-awaiting-review labels Aug 20, 2016

UK992 force-pushed the UK992:tidy-toml branch from 58276e2 to 69a4342 Aug 20, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 20, 2016

wafflespeanut added S-needs-squash and removed S-awaiting-review labels Aug 20, 2016

UK992 force-pushed the UK992:tidy-toml branch from 69a4342 to ad94d04 Aug 20, 2016

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

UK992 changed the title ~~WIP: Tidy config file~~ Tidy config file Aug 20, 2016

UK992 added 2 commits Aug 20, 2016

Tidy config file

0ad905a

Check for errors in tidy config file

Loading status checks…

74dba5c

UK992 force-pushed the UK992:tidy-toml branch from ad94d04 to 74dba5c Aug 20, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Aug 21, 2016

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

bors-servo merged commit 74dba5c into servo:master Aug 21, 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

servo / servo

Tidy config file #12877

Tidy config file #12877

UK992 commented Aug 15, 2016 •

edited by larsbergstrom

highfive commented Aug 15, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

edunham commented Aug 15, 2016 •

edited

UK992 commented Aug 15, 2016 •

edited

edunham commented Aug 16, 2016

wafflespeanut commented Aug 16, 2016

UK992 commented Aug 16, 2016

bors-servo commented Aug 17, 2016

wafflespeanut commented Aug 18, 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.

wafflespeanut commented Aug 20, 2016 •

edited

UK992 commented Aug 20, 2016

wafflespeanut commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

highfive commented Aug 21, 2016

wafflespeanut commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

servo / servo

Join GitHub today

Tidy config file #12877

Tidy config file #12877

Conversation

UK992 commented Aug 15, 2016 • edited by larsbergstrom

highfive commented Aug 15, 2016

This comment has been minimized.

edunham Aug 15, 2016 • edited

This comment has been minimized.

edunham Aug 15, 2016

This comment has been minimized.

UK992 Aug 15, 2016

This comment has been minimized.

edunham Aug 15, 2016 • edited

edunham commented Aug 15, 2016 • edited

UK992 commented Aug 15, 2016 • edited

edunham commented Aug 16, 2016

wafflespeanut commented Aug 16, 2016

UK992 commented Aug 16, 2016

bors-servo commented Aug 17, 2016

wafflespeanut commented Aug 18, 2016

This comment has been minimized.

wafflespeanut Aug 18, 2016 • edited

This comment has been minimized.

UK992 Aug 18, 2016

This comment has been minimized.

wafflespeanut Aug 18, 2016 • edited

This comment has been minimized.

UK992 Aug 18, 2016

This comment has been minimized.

wafflespeanut Aug 18, 2016

This comment has been minimized.

UK992 Aug 18, 2016

wafflespeanut commented Aug 20, 2016 • edited

UK992 commented Aug 20, 2016

wafflespeanut commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

highfive commented Aug 21, 2016

wafflespeanut commented Aug 21, 2016

bors-servo commented Aug 21, 2016

bors-servo commented Aug 21, 2016

UK992 commented Aug 15, 2016 •

edited by larsbergstrom

edunham Aug 15, 2016 •

edited

edunham Aug 15, 2016 •

edited

edunham commented Aug 15, 2016 •

edited

UK992 commented Aug 15, 2016 •

edited

wafflespeanut Aug 18, 2016 •

edited

wafflespeanut Aug 18, 2016 •

edited

wafflespeanut commented Aug 20, 2016 •

edited