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.

Sign up for GitHub

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

Jump to bottom

🏗 Add a --local_changes mode to gulp prettify #25093

Merged
merged 3 commits into from Oct 17, 2019
Merged

🏗 Add a --local_changes mode to gulp prettify #25093

merged 3 commits into from Oct 17, 2019

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented Oct 17, 2019 •
edited

PR highlights:

  • Adds a --local_changes mode to gulp prettify
  • Uses the globby package to convert an array of globs to a list of matching files (fast, widely used, actively maintained, can handle negative globs)
  • Refactors lintableFilesChanged() from lint.js into getFilesChanged(globs) in build-system/common/utils.js
  • Makes gulp pr-check run gulp prettify --local_changes
  • Makes the default pre-push hook run gulp prettify --local_changes

Coming up:

  • Stop using globs-to-files across build-system/ (hasn't been updated in 4 years / can't handle negative globs) and replace it with globby

Follow up to #25057 and #25085

@rsimha rsimha self-assigned this Oct 17, 2019
@amp-owners-bot amp-owners-bot bot requested a review from mrjoro October 17, 2019 04:35
@rsimha rsimha mentioned this pull request Oct 17, 2019
Merged
3 tasks
mrjoro
mrjoro approved these changes Oct 17, 2019
View reviewed changes
Copy link
Member

@mrjoro mrjoro left a comment

Choose a reason for hiding this comment

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

Giving approval for md file.

danielrozenberg
danielrozenberg approved these changes Oct 17, 2019
View reviewed changes
build-system/common/utils.js Outdated Show resolved Hide resolved
build-system/tasks/prettify.js Outdated Show resolved Hide resolved
rsimha
rsimha commented Oct 17, 2019
View reviewed changes
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.

Comments addressed.

@rsimha
Copy link
Contributor Author

rsimha commented Oct 17, 2019

@estherkim Good call on refactoring lintableFilesChanged(). This is ready for final review.

estherkim
estherkim approved these changes Oct 17, 2019
View reviewed changes
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.

LGTM, thanks.

@rsimha rsimha merged commit 6aa3bf2 into ampproject:master Oct 17, 2019
@rsimha rsimha deleted the 2019-10-16-PrettifyLocal branch October 17, 2019 17:44
@rsimha rsimha mentioned this pull request Oct 17, 2019
Merged
joshuarrrr pushed a commit to Parsely/amphtml that referenced this pull request Oct 22, 2019
@rsimha @joshuarrrr
🏗 Add a --local_changes mode to gulp prettify (ampproject#25093)
f6b2ab9
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
@rsimha
🏗 Add a --local_changes mode to gulp prettify (ampproject#25093)
582253f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielrozenberg danielrozenberg danielrozenberg approved these changes

@mrjoro mrjoro mrjoro approved these changes

@estherkim estherkim estherkim approved these changes

Assignees

@rsimha rsimha

Labels
cla: yes WG: infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rsimha @danielrozenberg @mrjoro @estherkim @googlebot