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

🏗🐛 Lint the contents of validator/ #15444

Merged
merged 3 commits into from
May 19, 2018
Merged

🏗🐛 Lint the contents of validator/ #15444

merged 3 commits into from
May 19, 2018

Conversation

rsimha
Copy link
Contributor

@rsimha rsimha commented May 19, 2018

This PR does the following:

  1. Enables eslint linting for the contents of validator/
  2. Adds a custom .eslintrc file for validator/ that supports node and jasmine code
  3. Auto-fixes as many lint errors as possible using gulp lint --files=validator/**/*.js --fix
  4. Switches all the lint rules for which failures remain to warning mode (so they can be fixed whenever possible)

@rsimha rsimha self-assigned this May 19, 2018
@rsimha
Copy link
Contributor Author

rsimha commented May 19, 2018

/to @Gregable @honeybadgerdontcare

honeybadgerdontcare
honeybadgerdontcare requested changes May 19, 2018
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

did you test that the validator chrome extension worked locally after this change?

validator/engine/htmlparser.js Outdated
'\/+(?!>))' + // e.g. "/"
'([^\\t\\r\\n /=>][^\\t\\r\\n =>]*|' + // e.g. "href"
'[^\\t\\r\\n =>]+[^ >]|' + // e.g. "/asdfs/asd"
'\/+(?!>))' + // e.g. "/"
Copy link
Contributor

Choose a reason for hiding this comment

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

boo. sad lint can't accept this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed by turning off the no-multi-spaces rule and reverting this (and other similar) changes.

honeybadgerdontcare
honeybadgerdontcare reviewed May 19, 2018
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

did you test that the validator chrome extension worked locally after this change?

honeybadgerdontcare
honeybadgerdontcare reviewed May 19, 2018
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

did you test that the validator chrome extension worked locally after this change?

@rsimha
Copy link
Contributor Author

rsimha commented May 19, 2018 •
edited
Loading

@honeybadgerdontcare PTAL.

I tested the chrome extension locally as follows:

  1. Installed bower and polybuild
  2. Ran validator/chromeextension/build_extension.sh and validator/chromeextension/package_extension.sh
  3. Checked that the extension in the zip file installed fine, and showed no validation errors with www.ampproject.org

Does this sound good?

honeybadgerdontcare
honeybadgerdontcare approved these changes May 19, 2018
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Sounds good.

@rsimha
Copy link
Contributor Author

rsimha commented May 19, 2018 •
edited
Loading

Cool, will merge, and rebase #15443 on this PR to confirm that they work well together.

@rsimha rsimha merged commit efac204 into ampproject:master May 19, 2018
@rsimha rsimha deleted the 2018-05-19-LintValidator branch May 19, 2018 16:21
@rsimha rsimha mentioned this pull request May 19, 2018
Merged
twifkak added a commit to twifkak/amphtml that referenced this pull request May 21, 2018
@twifkak
Linter tweaks for ampproject#15444
eb639c9
@twifkak twifkak mentioned this pull request May 21, 2018
Merged
honeybadgerdontcare pushed a commit that referenced this pull request May 22, 2018
@twifkak @honeybadgerdontcare
Validator rollup (#15470)
3ee6c34 
 - Revision bump for #14782
 - Linter tweaks for #15444
 - Test allowed_protocol:http and !allow_relative:false
 - Allow relative URLs for amp-story attributes
 - Restrict CSS to 1000 bytes per inline style
@rsimha rsimha mentioned this pull request May 26, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@Gregable Gregable Awaiting requested review from Gregable

Assignees

@rsimha rsimha

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

Successfully merging this pull request may close these issues.
3 participants
@rsimha @honeybadgerdontcare @googlebot