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

Update newspapers.com.js to use json string #1929

Open
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@pbinkley
Copy link
Contributor

commented Apr 12, 2019

Closes #1927

pbinkley added some commits Apr 12, 2019

@pbinkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2019

I'm not sure whether the attr() function is still needed, or what to do about the unused doc and url params that Travis doesn't like. I'd be happy to clean up further if someone can advise me.

@dstillman
Copy link
Member

left a comment

I didn't look at all of this, but some quick notes.

Show resolved Hide resolved newspapers.com.js Outdated
Show resolved Hide resolved newspapers.com.js Outdated
Show resolved Hide resolved newspapers.com.js Outdated
@pbinkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 13, 2019

Thanks for the suggestions. I've cleaned up the code in accordance with them. The JSON.parse() problem is weird: because the source string is stringified (with escaped slashes etc.), I have to apply JSON.parse twice to get it into an object. If there's a cleaner way to achieve this I'd love to learn it.

newspapers.com: Some clean-up
Remove unused functions, variables, duplicate tests
@adam3smith

This comment has been minimized.

Copy link
Collaborator

commented Apr 13, 2019

attr and text are no longer needed. The functions will work without the url parameter, but I'm pretty sure detectWeb does need doc so we'll need to escape the eslint rule there.
There was also something wonky about the tests -- somehow they had duplicated which caused the linter to fail.

Show resolved Hide resolved newspapers.com.js Outdated
@dstillman

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

The JSON.parse() problem is weird: because the source string is stringified (with escaped slashes etc.), I have to apply JSON.parse twice to get it into an object.

Sorry, that's right. Since we're treating the contents of this script as a string, the JSON string is itself stringified from the perspective of the translator (as opposed to the raw JS we see via View Source). There's no need for the typeof details === "string" conditional — it just always needs to be run through JSON.parse() twice.

Show resolved Hide resolved newspapers.com.js Outdated
Show resolved Hide resolved newspapers.com.js Outdated
@dstillman

This comment has been minimized.

Copy link
Member

commented on newspapers.com.js in ed0adaf Apr 17, 2019

This should be for (let metaTag of metaTags) {, and then you can just use metaTag instead of metaTags[metaTag]. (for … in is only for iterating objects, not arrays or array-like things.)

pbinkley added some commits Apr 17, 2019

@pbinkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

I think I've made all the requested changes, but let me know if there's more that needs doing.

@pbinkley

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Is any more work needed on this?

@pbinkley

This comment has been minimized.

Copy link
Contributor Author

commented May 9, 2019

I've just committed a fix for the case where the json string includes an apostrophe: the source escapes it with a backslash, which JSON.parse treats as an invalid character. I've added a test for this situation.

@adam3smith

This comment has been minimized.

Copy link
Collaborator

commented May 15, 2019

(I don't think this needs anything else; I'm just a bit underwater with reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.