Adding linting checks for buildbot_steps.yml #14051

leikahing · 2016-11-03T19:12:30Z

This pull request adds some tidy checks around YAML files, and specifically buildbot_steps.yml.

Tidy checks added:

YAML files are checked for well-formedness/parse-ability
Whether a YAML file has duplicate keys
Whether a buildbot_steps.yml file contains only mappings to list-of-strings.

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

There are tests for these changes OR

…ing checking for correct mappings and duplicate YAML keys. Added unit tests to test_tidy.py.

This change is

leikahing · 2016-11-03T19:25:29Z

This is still a work-in-progress per @aneeshusa. I'd like some feedback on if I'm taking the right approach with adding linting checks for buildbot_steps files, and YML files in general, along with using somewhat-descriptive messages.

wafflespeanut · 2016-11-04T05:59:11Z

r? @aneeshusa

bors-servo · 2016-11-07T15:22:19Z

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

aneeshusa · 2016-11-15T06:00:44Z

python/tidy/servo_tidy/tidy.py

+    if not file_name.endswith("buildbot_steps.yml"):
+        raise StopIteration
+
+    # YAML specification doesn't necessarily prevent


necessarily prevent -> explicitly disallow

aneeshusa · 2016-11-15T06:01:33Z

python/tidy/servo_tidy/tidy.py

+    return loader.construct_mapping(node, deep)
+
+
+class SafeYamlLoader(yaml.SafeLoader):


I don't see the benefit of this over using yaml.SafeLoader in the qualified form.

I guess for the unit tests, it's not necessary. Only reason to do this is because I do the SafeYamlLoader.add_constructor later on, which mutates the YAML loader. If I did this to the default yaml.SafeLoader, it would modify it globally.

Oh, I misread and thought we were calling add_constructor on an object returned by the Loader constructor, i.e.

loader = yaml.SafeLoader() loader.add_constructor(...) yaml.load(contents, loader=loader)

Since we're mutating the Loader itself, having the SafeYamlLoader seems like a better idea, so let's go back to that approach.

Oh, and please add a comment with this explanation for why we're making a subclass.

aneeshusa

Looks pretty good overall - just a few nits that I can see right now. The error messages are fine for now IMO.

aneeshusa · 2016-11-15T06:05:22Z

python/tidy/servo_tidy/tidy.py

+    # buildbot_steps.yml as it could lead to confusion
+    SafeYamlLoader.add_constructor(
+        yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
+        duplicate_key_yaml_constructor)


nit: closing paren on next line

leikahing · 2016-11-15T14:05:55Z

@aneeshusa Thanks for the feedback/review! Just updated my PR with changes addressing the issues you pointed out.

aneeshusa

Everything else looks good aside from these last two nits, so go ahead and squash everything into one commit. Please also update the commit message to follow the guidelines I mentioned on your other PR.

aneeshusa · 2016-11-15T17:10:11Z

python/tidy/servo_tidy/tidy.py

+
+def lint_buildbot_steps_yaml(mapping):
+    # Check for well-formedness of contents
+    # A well-formed buildbot_steps.yml should just be a map to list of strings


nit: remove just

leikahing · 2016-12-07T21:45:23Z

Review ping @aneeshusa

aneeshusa · 2016-12-07T22:57:31Z

Sorry about the delay! Looks good, but I'd still like to move back to subclassing yaml.SafeLoader (along with an explanatory comment) now that you've explained why.

leikahing · 2016-12-08T16:25:09Z

@aneeshusa Sorry, I didn't see the continuation of our code review discussion - GitHub apparently doesn't notify me of that.

I put the SafeLoader subclass back in with a comment, squashed my commits, and re-pushed.

aneeshusa · 2016-12-08T17:14:01Z

Thanks @birryree! @bors-servo r+

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

📌 Commit e28d1b2 has been approved by aneeshusa

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

⌛ Testing commit e28d1b2 with merge 5251f02...


        Auto merge of #14051 - birryree:tidy-check-buildbot-steps, r=aneeshusa

Adding linting checks for buildbot_steps.yml This pull request adds some tidy checks around YAML files, and specifically `buildbot_steps.yml`. Tidy checks added: * YAML files are checked for well-formedness/parse-ability * Whether a YAML file has duplicate keys * Whether a `buildbot_steps.yml` file contains only mappings to list-of-strings. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #13838 (github issue number if applicable).  - [X] There are tests for these changes OR  …ing checking for correct mappings and duplicate YAML keys. Added unit tests to test_tidy.py.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14051)

bors-servo · 2016-12-08T17:33:18Z

💔 Test failed - linux-dev

Ms2ger · 2016-12-08T17:37:45Z

python/tidy/servo_tidy/tidy.py:707: E302 expected 2 blank lines, found 1


        Add tidy linting checks for buildbot_steps.yml

This commit adds tidy checks for buildbot_steps.yml, as well as unit tests. These checks include: * Checking buildbot_steps.yml can be parsed by a YAML loader * buildbot_steps.yml does not contain duplicate keys * buildbot_steps.yml keys map to a list of strings

leikahing · 2016-12-09T01:38:14Z

Forgot to run test-tidy before I pushed the last commit. 😞

Pushed an update that resolves all that.

aneeshusa · 2016-12-09T01:39:40Z

@bors-servo r+

bors-servo · 2016-12-09T01:39:43Z

📌 Commit aceb60e has been approved by aneeshusa

bors-servo · 2016-12-09T01:39:45Z

⌛ Testing commit aceb60e with merge 21ad1c2...


        Auto merge of #14051 - birryree:tidy-check-buildbot-steps, r=aneeshusa

Adding linting checks for buildbot_steps.yml This pull request adds some tidy checks around YAML files, and specifically `buildbot_steps.yml`. Tidy checks added: * YAML files are checked for well-formedness/parse-ability * Whether a YAML file has duplicate keys * Whether a `buildbot_steps.yml` file contains only mappings to list-of-strings. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [x] These changes fix #13838 (github issue number if applicable).  - [X] There are tests for these changes OR  …ing checking for correct mappings and duplicate YAML keys. Added unit tests to test_tidy.py.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14051)

bors-servo · 2016-12-09T02:45:47Z

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

leikahing force-pushed the leikahing:tidy-check-buildbot-steps branch from 6a7aae3 to 7357244 Nov 3, 2016

highfive added the S-awaiting-review label Nov 3, 2016

highfive assigned aneeshusa Nov 4, 2016

highfive added the S-needs-rebase label Nov 7, 2016

leikahing force-pushed the leikahing:tidy-check-buildbot-steps branch from 7357244 to f247ca4 Nov 9, 2016

aneeshusa reviewed Nov 15, 2016

View changes

python/tidy/servo_tidy/tidy.py Outdated

if not file_name.endswith("buildbot_steps.yml"):

raise StopIteration

# YAML specification doesn't necessarily prevent

This comment has been minimized.

Sign in to view

aneeshusa Nov 15, 2016
Member

necessarily prevent -> explicitly disallow

aneeshusa reviewed Nov 15, 2016

View changes

aneeshusa requested changes Nov 15, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review S-needs-rebase labels Nov 15, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 15, 2016

aneeshusa requested changes Nov 15, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Nov 15, 2016

leikahing force-pushed the leikahing:tidy-check-buildbot-steps branch from e0373c6 to 81b257f Nov 16, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 16, 2016

leikahing changed the title ~~[WIP] Adding linting checks for buildbot_steps.yml~~ Adding linting checks for buildbot_steps.yml Nov 16, 2016

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Dec 7, 2016

leikahing force-pushed the leikahing:tidy-check-buildbot-steps branch from 81b257f to e28d1b2 Dec 8, 2016

highfive removed the S-needs-code-changes label Dec 8, 2016

highfive added the S-awaiting-review label Dec 8, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 8, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 8, 2016

leikahing force-pushed the leikahing:tidy-check-buildbot-steps branch from e28d1b2 to aceb60e Dec 9, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Dec 9, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 9, 2016

aneeshusa approved these changes Dec 9, 2016

View changes

bors-servo merged commit aceb60e into servo:master Dec 9, 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 Dec 9, 2016

leikahing deleted the leikahing:tidy-check-buildbot-steps branch Dec 10, 2016

servo / servo

Adding linting checks for buildbot_steps.yml #14051

Adding linting checks for buildbot_steps.yml #14051

leikahing commented Nov 3, 2016 •

edited by larsbergstrom

leikahing commented Nov 3, 2016

wafflespeanut commented Nov 4, 2016

bors-servo commented Nov 7, 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.

aneeshusa left a comment

This comment has been minimized.

leikahing commented Nov 15, 2016

aneeshusa left a comment

This comment has been minimized.

leikahing commented Dec 7, 2016

aneeshusa commented Dec 7, 2016 •

edited

leikahing commented Dec 8, 2016

aneeshusa commented Dec 8, 2016

bors-servo commented Dec 8, 2016

bors-servo commented Dec 8, 2016

bors-servo commented Dec 8, 2016

Ms2ger commented Dec 8, 2016

leikahing commented Dec 9, 2016

aneeshusa commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

		return loader.construct_mapping(node, deep)


		class SafeYamlLoader(yaml.SafeLoader):

servo / servo

Join GitHub today

Adding linting checks for buildbot_steps.yml #14051

Adding linting checks for buildbot_steps.yml #14051

Conversation

leikahing commented Nov 3, 2016 • edited by larsbergstrom

leikahing commented Nov 3, 2016

wafflespeanut commented Nov 4, 2016

bors-servo commented Nov 7, 2016

This comment has been minimized.

aneeshusa Nov 15, 2016

This comment has been minimized.

aneeshusa Nov 15, 2016

This comment has been minimized.

leikahing Nov 15, 2016

This comment has been minimized.

aneeshusa Nov 15, 2016

This comment has been minimized.

aneeshusa Nov 15, 2016

aneeshusa left a comment

This comment has been minimized.

aneeshusa Nov 15, 2016

leikahing commented Nov 15, 2016

aneeshusa left a comment

This comment has been minimized.

aneeshusa Nov 15, 2016

leikahing commented Dec 7, 2016

aneeshusa commented Dec 7, 2016 • edited

leikahing commented Dec 8, 2016

aneeshusa commented Dec 8, 2016

bors-servo commented Dec 8, 2016

bors-servo commented Dec 8, 2016

bors-servo commented Dec 8, 2016

Ms2ger commented Dec 8, 2016

leikahing commented Dec 9, 2016

aneeshusa commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

bors-servo commented Dec 9, 2016

leikahing commented Nov 3, 2016 •

edited by larsbergstrom

aneeshusa commented Dec 7, 2016 •

edited