Skip to content

Conversation

@osaid-r
Copy link
Contributor

@osaid-r osaid-r commented May 26, 2020

Fixes #92
Once merged this would return Open Graph array when uniform is set True

@codecov
Copy link

codecov bot commented May 26, 2020

Codecov Report

Merging #138 into master will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #138      +/-   ##
==========================================
+ Coverage   87.78%   88.06%   +0.27%     
==========================================
  Files          11       11              
  Lines         475      486      +11     
  Branches      103      108       +5     
==========================================
+ Hits          417      428      +11     
  Misses         52       52              
  Partials        6        6              
Impacted Files Coverage Δ
extruct/_extruct.py 73.33% <100.00%> (ø)
extruct/uniform.py 94.44% <100.00%> (+1.00%) ⬆️
extruct/rdfa.py 100.00% <0.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...be85256. Read the comment docs.

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 for the PR @ragnerok , some general feedback:

  • could you please use 4 spaces for indentation, like in the rest of the project? Both are fine options but it's best to have the same setting through the whole project, and we'd like to stick to 4.
  • could you please add tests which cover the new behavior?

@osaid-r
Copy link
Contributor Author

osaid-r commented May 27, 2020

@lopuhin I am sorry my text editor was set to 2 spaces, I'll fix it. I'll add a few test cases as well.

* Fixed some style issues
* Added testing for og_array
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 @ragnerok left some comments

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 @ragnerok I think PR is in great shape and almost ready to get merged - please check remaining comments.

* Removed non_empty_props
* Added test that checks duplicated and empty properties
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.

Looks great, thanks @ragnerok ! I think PR is ready to be merged, leaving it open for a bit in case someone else wants to review.

Co-authored-by: Adrián Chaves <adrian@chaves.io>
@lopuhin lopuhin merged commit a64ce58 into scrapinghub:master Jun 8, 2020
@lopuhin
Copy link
Member

lopuhin commented Jun 8, 2020

Thanks @ragnerok and thanks for review @Gallaecio 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for Open Graph Arrays

3 participants