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
Implement validation using Cloudflare's AMP linter API endpoint #59
base: master
Are you sure you want to change the base?
Conversation
@ale-rt are you using AMP? do you want to help reviewing this? @kakshay21 this is what I was expecting just as a beginning. |
e4b0da5
to
59a89c7
Compare
@hvelarde I am interested in the project but unluckily I had no chance to use it and inspect the code in detail. |
"""Validate @@amp view using Cloudflare's AMP linter API endpoint.""" | ||
request = obj.REQUEST | ||
|
||
if event.status['review_state'] not in ('published', ): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if event.status['review_state'] != 'published':
return | ||
|
||
# remove the scheme from the URL | ||
url = ''.join(urlparse(obj.absolute_url())[1:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
url = obj.absolute_url() # http://site/myobject
url_no_scheme = ''.join(urlparse(url))[1:] # site/myobject
# remove the scheme from the URL | ||
url = ''.join(urlparse(obj.absolute_url())[1:]) | ||
|
||
r = requests.get('https://amp.cloudflare.com/q/' + url + '/@@amp') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r = requests.get('https://amp.cloudflare.com/q/{}/@@amp'.format(url_no_scheme))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pointed some code review. But it looks good the way it is
I have finished 1 of the 7 things needed here; a review at this point is useless.
logger.info(msg) | ||
api.portal.show_message(message=msg, request=request, type='info') | ||
else: | ||
msg = u'Not a valid AMP page' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not include the error in the warning message?
for error in validation['errors']:
msg = msg + str(error)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I get it it would be difficult in writing the test case then right? @hvelarde
WIP
closes #16