Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upUpdate newspapers.com.js to use json string #1929
Conversation
pbinkley
added some commits
Apr 12, 2019
This comment has been minimized.
This comment has been minimized.
I'm not sure whether the |
dstillman
requested changes
Apr 12, 2019
I didn't look at all of this, but some quick notes. |
This comment has been minimized.
This comment has been minimized.
Thanks for the suggestions. I've cleaned up the code in accordance with them. The |
This comment has been minimized.
This comment has been minimized.
attr and text are no longer needed. The functions will work without the |
adam3smith
reviewed
Apr 14, 2019
newspapers.com.js Outdated
This comment has been minimized.
This comment has been minimized.
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 |
dstillman
reviewed
Apr 16, 2019
newspapers.com.js Outdated
dstillman
reviewed
Apr 16, 2019
newspapers.com.js Outdated
This comment has been minimized.
This comment has been minimized.
This should be |
pbinkley
added some commits
Apr 17, 2019
This comment has been minimized.
This comment has been minimized.
I think I've made all the requested changes, but let me know if there's more that needs doing. |
This comment has been minimized.
This comment has been minimized.
Is any more work needed on this? |
This comment has been minimized.
This comment has been minimized.
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. |
This comment has been minimized.
This comment has been minimized.
(I don't think this needs anything else; I'm just a bit underwater with reviews. |
pbinkley commentedApr 12, 2019
Closes #1927