scrapy / scurl

Performance-focused replacement for Python urllib
Apache License 2.0
21 stars 6 forks source link

Support different versions of Python #32

Closed malloxpb closed 6 years ago

malloxpb commented 6 years ago

In this PR, I added more py env to the library to make sure the library can support multiple python versions. Right now, it passes py27, py34, py35 and py36 tests. I will make more updates to this PR soon. For py34, the test file for urlparse is a little bit different but I just copied that from the 3.4 branch

codecov[bot] commented 6 years ago

Codecov Report

Merging #32 into master will decrease coverage by 0.03%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #32      +/-   ##
==========================================
- Coverage   88.49%   88.46%   -0.04%     
==========================================
  Files           2        2              
  Lines         313      312       -1     
==========================================
- Hits          277      276       -1     
  Misses         36       36
Impacted Files Coverage Δ
scurl/canonicalize.pyx 86.45% <ø> (-0.14%) :arrow_down:
scurl/cgurl.pyx 89.35% <100%> (ø) :arrow_up:
lopuhin commented 6 years ago

Won't existing python 3 tests pass for python 3.4? If they will, I think it will be easier to support less variants of test cases.

malloxpb commented 6 years ago

Hey @lopuhin , the existing python3 test has some methods that were not implemented in python3.4. The python3.4 test could not import this, and this. What should I do in this test file in order to pass the test on py3.4, @lopuhin ? I know that the quote_via argument is not implemented in urlencode() py34

malloxpb commented 6 years ago

Ohh I can skip the test based on the version of python... I will work on that now!

malloxpb commented 6 years ago

Hey @lopuhin, I have deleted the py34 test and skip the test that has quote_via arg in urlencode func on python34 only! Now it passes all the tests 😄

malloxpb commented 6 years ago

Hey @kmike @lopuhin , I have a quick question. I noticed that Cython cannot compile on pypy and pypy3 if linetraced is defined in the pyx files, and linetraced is required for the coverage profiling. So what should I do in this situation... Please let me know when you have time 😄

lopuhin commented 6 years ago

@nctl144 as far as I understand, linetrace in the source is disabled by default, and must be explicitly enabled: https://cython.readthedocs.io/en/latest/src/tutorial/profiling_tutorial.html#enabling-line-tracing - so I wonder if it will work on pypy if you disable coverage report, by using a custom command in tox?

lopuhin commented 6 years ago

Ah sorry, actually upon reading the source code I see that distutils: define_macros=CYTHON_TRACE=1 is right in the .pyx sources - is it possible to move it outside? This will not only benefit pypy, but I think will make the code run faster under CPython, because this line always enables tracing, and it's said to be slow in the docs.

malloxpb commented 6 years ago

@lopuhin I think this PR is ready :)

malloxpb commented 6 years ago

Thank you Konstantin!