Skip to content

Conversation

@kmike
Copy link
Member

@kmike kmike commented Nov 1, 2018

  • added Python 3.7 to tests
  • fix Python 3.7 warnings
  • improve DeprecationWarning, so that it points to a caller code

* added Python 3.7 to tests
* fix Python 3.7 warnings
* improve DeprecationWarning, so that it points to a caller code
@kmike kmike requested a review from lopuhin November 1, 2018 19:55
@codecov
Copy link

codecov bot commented Nov 1, 2018

Codecov Report

Merging #97 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master     #97   +/-   ##
======================================
  Coverage    88.7%   88.7%           
======================================
  Files          11      11           
  Lines         434     434           
  Branches       89      89           
======================================
  Hits          385     385           
  Misses         44      44           
  Partials        5       5
Impacted Files Coverage Δ
extruct/_extruct.py 78.84% <ø> (ø) ⬆️
extruct/tool.py 100% <100%> (ø) ⬆️
extruct/jsonld.py 100% <100%> (ø) ⬆️

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 18f41f1...f79972f. 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.

Looks good, thanks @kmike !

result.update(extruct.extract(resp.content,
url=url,
result.update(extruct.extract(resp.content,
base_url=url, # FIXME: use base url
Copy link
Member

Choose a reason for hiding this comment

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

One more reason to deprecate the tool - I was going to suggest using get_base_url from w3lib, but it seems this would require more extra code to handle encodings correctly (and we are already throwing encoding information away here by not using headers).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, actually I started adding get_base_url, then realized encodings need to be handled, etc., removed that, and just added FIXME comment instead :)

@lopuhin lopuhin merged commit 477e2ef into master Nov 2, 2018
@lopuhin lopuhin deleted the py37 branch November 2, 2018 08:09
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.

3 participants