Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace get_func_args #5885

Merged
merged 10 commits into from
Apr 13, 2023
Merged

Replace get_func_args #5885

merged 10 commits into from
Apr 13, 2023

Conversation

serhii73
Copy link
Collaborator

@serhii73 serhii73 commented Apr 4, 2023

Close #5872

@serhii73 serhii73 changed the title Replace get_func_args [WIP] Replace get_func_args Apr 4, 2023
@codecov
Copy link

codecov bot commented Apr 4, 2023

Codecov Report

Merging #5885 (6a7edd2) into master (8045d7e) will decrease coverage by 0.05%.
The diff coverage is 92.59%.

❗ Current head 6a7edd2 differs from pull request most recent head e532f7e. Consider uploading reports for the commit e532f7e to get more accurate results

@@            Coverage Diff             @@
##           master    #5885      +/-   ##
==========================================
- Coverage   88.85%   88.81%   -0.05%     
==========================================
  Files         162      162              
  Lines       11057    11114      +57     
  Branches     1801     1802       +1     
==========================================
+ Hits         9825     9871      +46     
- Misses        954      961       +7     
- Partials      278      282       +4     
Impacted Files Coverage Δ
scrapy/mail.py 77.77% <0.00%> (-0.88%) ⬇️
scrapy/pipelines/files.py 72.00% <50.00%> (+0.09%) ⬆️
scrapy/utils/misc.py 96.52% <60.00%> (-1.35%) ⬇️
scrapy/utils/python.py 89.67% <80.95%> (-1.30%) ⬇️
scrapy/signalmanager.py 81.81% <83.33%> (+1.81%) ⬆️
scrapy/core/engine.py 83.68% <90.90%> (-0.51%) ⬇️
scrapy/statscollectors.py 89.28% <92.85%> (-2.88%) ⬇️
scrapy/core/scheduler.py 93.23% <93.75%> (-0.52%) ⬇️
scrapy/core/downloader/__init__.py 92.80% <100.00%> (+0.05%) ⬆️
scrapy/core/scraper.py 84.94% <100.00%> (+0.33%) ⬆️
... and 11 more

@wRAR
Copy link
Member

wRAR commented Apr 4, 2023

@serhii73 serhii73 changed the title [WIP] Replace get_func_args Replace get_func_args Apr 7, 2023
@wRAR
Copy link
Member

wRAR commented Apr 7, 2023

We had some discussions regarding the operator.itemgetter case and so far it's not clear to me how to handle it in the best way. On CPython inspect.signature(operator.itemgetter(2)) raises ValueError: callable operator.itemgetter(2) is not supported by signature and my initial suggestion was to catch this exception and return []. I've just checked this on PyPy and it returns <Signature (obj)>, so it works there. So it seems that we still need separate checks in the test for CPython and PyPy, where PyPy will assert ["obj"] like it does now, and CPython will expect []?

Specifically omitting operator.itemgetter like in this PR is not a good idea because get_func_args() previously worked for it on PyPy and because there should be other cases that doesn't work on CPython. Not handling the exception is also worse than returning []. So keeping separate test conditions seems the best approach to me.

@serhii73 serhii73 changed the title Replace get_func_args [WIP] Replace get_func_args Apr 10, 2023
serhii73 and others added 2 commits April 12, 2023 16:57
Co-authored-by: Adrián Chaves <adrian@chaves.io>
@serhii73 serhii73 changed the title [WIP] Replace get_func_args Replace get_func_args Apr 12, 2023
Co-authored-by: Andrey Rakhmatullin <wrar@wrar.name>
@Gallaecio Gallaecio merged commit c2a3197 into scrapy:master Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_func_args does not fully work in CPython
3 participants