alecxe / scrapy-fake-useragent

Random User-Agent middleware based on fake-useragent

MIT License

686 stars 98 forks source link

Support for different UA providers #28

Closed 0xfede7c8 closed 4 years ago

0xfede7c8 commented 4 years ago

Still a few details missing, like fixing the tests, and actually smoke test it.

@alecxe you can check how it is looking

Feel free to ask anything and suggest any changes.

0xfede7c8 commented 4 years ago

Applied changes of review. Still missing the tests.

@alecxe I fixed the README. Feel free to clone the fork and push to this branch whatever changes to the readme you want.

alecxe commented 4 years ago

This is looking awesome so far @0xfede7c8!

0xfede7c8 commented 4 years ago

Should I change anything to the tests? I thought I did but as we are mantaining compatibility it should work right? BTW, how should I run the tests to try?

alecxe commented 4 years ago

@0xfede7c8 yeah, it's important to see that the existing tests pass as is.

Apologies as we don't really have test/dev environment setup instructions for contributors here. You should be able to just call pytest after installing dev requirements with pip install -r dev-requirements.txt. pytest would use pytest.ini options and auto-discover tests automatically. Lmk if it works.

0xfede7c8 commented 4 years ago

Tests passing. Fixed some stuff that is discovered while testing (i.e: naming collision with Faker and python2 compatibility).

codecov[bot] commented 4 years ago

Codecov Report

Merging #28 into master will increase coverage by 2.34%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #28      +/-   ##
==========================================
+ Coverage   95.65%   98.00%   +2.34%     
==========================================
  Files           1        2       +1     
  Lines          46      100      +54     
==========================================
+ Hits           44       98      +54     
  Misses          2        2

Impacted Files	Coverage Δ
scrapy_fake_useragent/middleware.py	`96.72% <100.00%> (+1.06%)`	:arrow_up:
scrapy_fake_useragent/providers.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 3e5ed91...d5d7688. Read the comment docs.

0xfede7c8 commented 4 years ago

Ok, it seems that I need to make some tests to cover the new providers.

alecxe commented 4 years ago

The tests on travis fail (https://travis-ci.org/github/alecxe/scrapy-fake-useragent), but they pass locally. I suspect the mock for fake-useragent does not work there for some reason and, hence, fake-useragent fails to get the user-agents. I'll keep trying to make it work but lmk if you have ideas what is going on. Thanks.

0xfede7c8 commented 4 years ago

fake-useragent can make use of a cache in /tmp/something. I can be that? Try deleting that and running locally again.

alecxe commented 4 years ago

fake-useragent can make use of a cache in /tmp/something. I can be that? Try deleting that and running locally again.

Bingo. Yes, it was an incorrect mock that was then cached and reused :) Thanks!

All tests passed. Does it all look good to you and would work for your use case? Once I get a thumbs up from you, I'll go ahead and merge and upload to PyPI - lmk.

Thanks a lot for doing this!

0xfede7c8 commented 4 years ago

It's good for my use case! My pleasure, thanks for the help!