search

scrapy / parsel

Parsel lets you extract data from XML/HTML documents using XPath or CSS selectors
BSD 3-Clause "New" or "Revised" License
1.15k stars 146 forks source link

Add PEP 561-style type information #218

Closed pcorpet closed 3 years ago

pcorpet commented 3 years ago

Note that I'm not sure how to correctly add the files in the package. The *.pyi files need to be installed, so as the empty py.typed file.

As for the types, we've been using them for a while, with scrapy types as well. They are not complete but it's a start, and there's no reason for those types to live in our repo when they can benefit others.

codecov[bot] commented 3 years ago

Codecov Report

Merging #218 (9de2cd2) into master (56bec18) will not change coverage. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #218   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          286       291    +5     
  Branches        51        51           
=========================================
+ Hits           286       291    +5
Impacted Files Coverage Δ
parsel/__init__.py 100.00% <100.00%> (ø)
parsel/selector.py 100.00% <100.00%> (ø)
parsel/utils.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 56bec18...9de2cd2. Read the comment docs.

Gallaecio commented 3 years ago

I was not aware of these pyi files, but could you instead include the typing hints into the actual files? That’s the way we are doing it in Scrapy, and I think it makes things easier (e.g. if you change a function you only need to edit its file, not also the corresponding type file).

pcorpet commented 3 years ago

I'm happy to go that way as it's indeed easier to maintain.

Note that it enforces to use python 3, and I have no clue whether parsel still support python 2.

Gallaecio commented 3 years ago

You are right, we still support Python 2 :facepalm:.

Maybe it’s time for us to “fix” that :slightly_smiling_face:

Gallaecio commented 3 years ago

Fixed, please merge/rebase.

pcorpet commented 3 years ago

Done. Note that the annotations I've added are not enough to type parsel entirely, it's only typing the public API so that other typed code can depend on it properly.

Gallaecio commented 3 years ago

Note that the annotations I've added are not enough to type parsel entirely, it's only typing the public API so that other typed code can depend on it properly.

That’s perfectly fine. It’s definitely an improvement over not having any typing hints at all.

Could you also add mypy to the CI? It should only require editing tox.ini and .github/workflows/checks.yml. See similar files in https://github.com/scrapy/itemadapter for reference.

pcorpet commented 3 years ago

Alright, I've added mypy. Few things: