Skip to content

feat: add parser for JSON with JS comment #137

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 7 commits into from
Sep 1, 2020

Conversation

Kiollpt
Copy link

@Kiollpt Kiollpt commented May 11, 2020

I encountered the issue of parsing json-ld with JS comments. Finally, I found the jstyleson package that could solve.

Reproduce issue:

1. comment the key-value pair 
2. comment occurs after the key-value pair 

{
 '\t//"interactionCount": "", // 文章互動數 \r\n'
'\t\t"height": "1000", // 高度, 有預設或是的高度\r\n'
}

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #137 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #137      +/-   ##
==========================================
+ Coverage   87.78%   87.81%   +0.02%     
==========================================
  Files          11       11              
  Lines         475      476       +1     
  Branches      103      103              
==========================================
+ Hits          417      418       +1     
  Misses         52       52              
  Partials        6        6              
Impacted Files Coverage Δ
extruct/jsonld.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0648f1a...8e5a603. Read the comment docs.

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Do not forget setup.py.

@Kiollpt Kiollpt force-pushed the parse-js-comment-json branch from 2aef0d4 to 5bb1a87 Compare May 15, 2020 08:38
Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Could you add a least one test that covers a scenario that was not supported by the old code and now works thanks to your changes?

@Kiollpt Kiollpt requested a review from Gallaecio May 15, 2020 12:48
@Gallaecio
Copy link
Member

Can this library handle #53? Otherwise, it may make sense to look into using https://github.com/codecobblers/dirtyjson instead as suggested by @whalebot-helmsman at #126 (comment)

@Kiollpt
Copy link
Author

Kiollpt commented May 16, 2020

After quick testing, both library can not handle #53.
Additionally, I found it would generate AttributedDict,not default dict after loads().

@Gallaecio
Copy link
Member

Let me know if I can help with the tests.

@Kiollpt
Copy link
Author

Kiollpt commented May 20, 2020

yes.
And I already added the unit test for this change

Copy link
Member

@Gallaecio Gallaecio left a comment

Choose a reason for hiding this comment

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

Awesome!

@Gallaecio Gallaecio mentioned this pull request Jun 1, 2020
Copy link
Member

@lopuhin lopuhin left a comment

Choose a reason for hiding this comment

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

Thanks @Kiollpt , looks great 👍

@lopuhin lopuhin merged commit 1c310c5 into scrapinghub:master Sep 1, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants