scrapy / itemloaders

Library to populate items using XPath and CSS with a convenient API

BSD 3-Clause "New" or "Revised" License

45 stars 16 forks source link

Cleanup #1

Closed ejulio closed 4 years ago

ejulio commented 4 years ago

Draft while removing dependencies from scrapy

kmike commented 4 years ago

A meta-question about this PR. https://github.com/scrapy/scrapy/pull/3881 is going to be merged soon (hopefully); it makes quite a serious change to the itemloaders, adding dataclass support. What's the plan of pulling changes to this repo? Should it be done because this PR is merged, or is there a way to do it afterwards (maybe with rebases)?

ejulio commented 4 years ago

@kmike probably, the best approach, would be to merge those changes to master in this repo and then merge this PR. That could help keeping the history sane and avoid some merges

kmike commented 4 years ago

hey @ejulio! I think docs may need some files around them, to be buildable.

ejulio commented 4 years ago

@kmike, regarding the missing docs files, I just copied the text ones. As probably the ones for building are quite standard, so not sure about keeping the history of those. But I can add them as well

kmike commented 4 years ago

@ejulio keeping history is not important for other doc files, but there should be away to build docs (a sphinx project); docs should be built by readthedocs.org eventually.

ejulio commented 4 years ago

@kmike I'm working on it in another PR https://github.com/ejulio/itemloaders/pull/4

codecov[bot] commented 4 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@81f54fb). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master       #1   +/-   ##
=========================================
  Coverage          ?   98.00%           
=========================================
  Files             ?        4           
  Lines             ?      251           
  Branches          ?        0           
=========================================
  Hits              ?      246           
  Misses            ?        5           
  Partials          ?        0

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 81f54fb...81f54fb. Read the comment docs.

kmike commented 4 years ago

Hey @ejulio! The PR looks good to me; I think we can update master to match Scrapy, merge master to this branch (ensuring that changes are preserved), and then merge this branch to master.