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

Initial contribution of the validator #951

Merged
merged 1 commit into from
Nov 18, 2015
Merged

Initial contribution of the validator #951

merged 1 commit into from
Nov 18, 2015

Conversation

powdercloud
Copy link
Contributor

Written by @Gregable and @powdercloud of Google,
with credits given to original versions for the
css parser/tokenizer and the html parser in the file headers.

This is the validator which is already used by the amphtml developer mode;
see also https://github.com/ampproject/amphtml#the-amp-validator

README.md describes how to build this; there's currently only a minimal
ad-hoc build script, build.py, which should work for Ubuntu 14.

We still need to contribute our tests (we do have tests), so this is
only a first step.

@cramforce
Copy link
Member

@erwinmombay Could you also take a look.

@@ -0,0 +1,201 @@
Apache License
Copy link
Member

Choose a reason for hiding this comment

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

No need to repeat here.

@cramforce
Copy link
Member

Does it make sense to also check in the generated JSON, so we can use the validator without the complicated build process?

@erwinmombay
Copy link
Member

@cramforce will do

@powdercloud
Copy link
Contributor Author

I'd like to not check this generated stuff in initially, just so that there is a clean checkin. I think it's better to figure out what you need to have checked in once it's clear how you want to use it.

To see what this generates (actually Javascript, not json), I stuck it up as well.
https://github.com/powdercloud/amphtml/blob/codegen/validator/codegen/validator-generated.js

@cramforce
Copy link
Member

Definitely, lets get this in quickly. I'm fine with merging with the 2 comments above resolved.

"name": "amp_validator",
"version": "0.1.0",
"description": "Validator for AMP HTML (www.ampproject.org)",
"main": "index.js",
Copy link
Member

Choose a reason for hiding this comment

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

is there actually an index.js file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is one, but not included in this PR. The purpose of package.json is to let build.py fetch the dependencies, so index.js isn't used for anything, and build.py contains a snippet which generates its own main method, available (when built) as ./codegen/validate. I'll remove this line and see how it goes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this "main": entry does nothing, so I got rid of it.

@erwinmombay
Copy link
Member

@cramforce should we whitelist validator/**/*.* for presubmit? (i already assume we need to skip it for lint checks temporarily)

@cramforce
Copy link
Member

@erwinmombay whitelisting everything but the license check is fine (or at least make sure we pass that check). When we make this the master repo for the validator we can bring everything up to pass.

<link rel="canonical" href="./regular-html-version.html" />
<meta name="viewport" content="width=device-width,minimum-scale=1">
<style>body {opacity: 0}</style><noscript><style>body {opacity: 1}</style></noscript>
<script src="https://ssl.gstatic.com/amphtml/v0.js" async></script>
Copy link
Member

Choose a reason for hiding this comment

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

move async as the first prop for best practices. (i can make this change later, just marking it for now)

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

@powdercloud
Copy link
Contributor Author

@cramforce removed the two files.

@erwinmombay
Copy link
Member

@cramforce created #953

@powdercloud of Google, with credits given to original versions for the
css parser/tokenizer and the html parser in the file headers.

This is the validator which is already used by the amphtml developer mode;
see also https://github.com/ampproject/amphtml#the-amp-validator

README.md describes how to build this; there's currently only a minimal
ad-hoc build script, build.py, which should work for Ubuntu 14.

We still need to contribute our tests (we do have tests), so this is
only a first step.
@powdercloud
Copy link
Contributor Author

Looks like the checks are green now, thanks to you.
If there's something else that I need to be doing to this pull request, please lemme know.

@erwinmombay
Copy link
Member

LGTM, deferring to @cramforce for final LGTM

@cramforce
Copy link
Member

LGTM. Merging.

cramforce added a commit that referenced this pull request Nov 18, 2015
Initial contribution of the validator
@cramforce cramforce merged commit 1a67efc into ampproject:master Nov 18, 2015
@henel677 henel677 mentioned this pull request Jan 21, 2020
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.

4 participants