Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🏗✨ Check non-JS files for formatting errors using prettier during local and Travis PR checks #25057

Merged
merged 10 commits into from Oct 16, 2019
Merged

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Oct 15, 2019

PR highlights:

  • Adds a new task gulp prettify to check non-JS files for formatting errors using prettier
  • Adds a new task gulp check-owners to check OWNERS files for correctness (with a placeholder check until [owners] implement blocking Travis check to validate modified OWNERS.yaml syntax amp-github-apps#281 is fixed)
  • Removes the old task gulp json-syntax since it will be replaced by gulp prettify for .json files
  • Renames build-system/tasks/json-syntax.js to build-system/tasks/caches-json.js now that it only checks caches.json for correctness
  • Updates the build target logic used by the local and Travis PR check workflow to account for OWNERS files (so all tests aren't run for PRs that only touch OWNERS files)
  • Adds the newly introduced checks to the local and Travis PR check workflows
  • Moves some common functionality used by tasks like lint, prettify, etc. to a new file build-system/common/utils.js

Coming up:

Usage:

# Check all non-JS files (that can be formatted with prettier)
gulp prettify

# Check and fix all non-JS files
gulp prettify --fix

# Check a subset of non-JS files
gulp prettify --files "<glob>"
# Check all `OWNERS` files
gulp check-owners

# Check and fix all OWNERS files
gulp check-owners --fix

# Check a subset of OWNERS files
gulp check-owners --files "build-system/**/OWNERS"

Fixes #24936
Fixes #24942

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for figuring this out. Left a couple comments, approving to unblock

.prettierrc Outdated Show resolved Hide resolved
build-system/pr-check/build-targets.js Show resolved Hide resolved
build-system/pr-check/build-targets.js Show resolved Hide resolved
build-system/pr-check/checks.js Show resolved Hide resolved
build-system/tasks/check-owners.js Outdated Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed PR comments.

.prettierrc Outdated Show resolved Hide resolved
build-system/pr-check/build-targets.js Show resolved Hide resolved
build-system/pr-check/build-targets.js Show resolved Hide resolved
build-system/pr-check/checks.js Show resolved Hide resolved
build-system/tasks/check-owners.js Outdated Show resolved Hide resolved
build-system/pr-check/build-targets.js Outdated Show resolved Hide resolved
build-system/tasks/check-owners.js Show resolved Hide resolved
@rsimha rsimha changed the title 🏗✨ Check OWNERS files during local and Travis PR checks 🏗✨ Check non-JS files for formatting errors using prettier during local and Travis PR checks Oct 15, 2019
@rsimha
Copy link
Contributor Author

rsimha commented Oct 15, 2019

@estherkim I've rewritten this PR based on our discussion. The gulp prettify task will check for formatting (and optionally fix), while the gulp check-owners task can be used to check OWNERS files for correctness.

build-system/tasks/check-owners.js Outdated Show resolved Hide resolved
build-system/tasks/prettify.js Show resolved Hide resolved
build-system/tasks/prettify.js Show resolved Hide resolved
build-system/tasks/prettify.js Show resolved Hide resolved
build-system/tasks/prettify.js Outdated Show resolved Hide resolved
build-system/tasks/prettify.js Show resolved Hide resolved
build-system/tasks/prettify.js Show resolved Hide resolved
build-system/tasks/check-owners.js Show resolved Hide resolved
Copy link
Contributor Author

@rsimha rsimha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rcebulko All comments addressed.

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment re: [].forEach(async .. => ...), approving to unblock

build-system/tasks/check-owners.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@estherkim estherkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the work on this! And thanks for clarifying the changes in the PR description.

@rsimha
Copy link
Contributor Author

rsimha commented Oct 16, 2019

Thanks for the reviews. Looks like the latest rebase caught some bad formatting: https://travis-ci.org/ampproject/amphtml/jobs/598788352#L412

I'll rebase, fix, and check this in once Travis is green.

@rsimha rsimha merged commit 10e8da8 into ampproject:master Oct 16, 2019
@rsimha rsimha deleted the 2019-10-10-GulpPrettier branch October 16, 2019 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't run all PR checks for OWNERS-only changes Update Prettier config to format OWNERS files
4 participants