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

[MRG+1] Add ability to use FormRequest in contracts #3383

Merged
merged 3 commits into from Sep 5, 2018

Conversation

@StasDeep
Copy link
Contributor

@StasDeep StasDeep commented Aug 11, 2018

With current implementation of contracts, it is basically not possible to create a generic @form contract, but I think we should at least allow to add formdata in custom contracts.

Closes #3382.

@codecov
Copy link

@codecov codecov bot commented Aug 11, 2018

Codecov Report

Merging #3383 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3383      +/-   ##
==========================================
+ Coverage   84.46%   84.48%   +0.01%     
==========================================
  Files         167      167              
  Lines        9362     9369       +7     
  Branches     1390     1392       +2     
==========================================
+ Hits         7908     7915       +7     
  Misses       1199     1199              
  Partials      255      255
Impacted Files Coverage Δ
scrapy/contracts/__init__.py 80.5% <100%> (+0.86%) ⬆️
scrapy/spidermiddlewares/offsite.py 100% <0%> (ø) ⬆️
@kmike
Copy link
Member

@kmike kmike commented Aug 17, 2018

Hey @StasDeep! What do you think about adding request_cls attribute to Contract instead, so that one can define a contract which uses FormRequest, SplashRequest or any other Request class? I don't quite like hardcoding formdata argument.

@StasDeep
Copy link
Contributor Author

@StasDeep StasDeep commented Aug 19, 2018

Hi @kmike! Thanks for the idea, that sounds much better indeed. I've implemented request_cls feature. Could you please review it?

name = 'custom_form'

def adjust_request_args(self, args):
args['request_cls'] = FormRequest

This comment has been minimized.

@kmike

kmike Aug 20, 2018
Member

I was thinking about doing it in a different way:

class CustomFormContract(Contract):
    name = 'custom_form'
    request_cls = FormRequest

    def adjust_request_args(self, args):
         args['formdata'] = {'name': 'scrapy'}
         return args

Not passing request_cls in args looks cleaner to me, because request_cls is not really a keyword argument passed to a request initializer.

That said, if it is not an request arg, we'd need some rule to determine which class to use if there are several contracts in a chain. Something like "take last explicitly defined request_cls".

@StasDeep
Copy link
Contributor Author

@StasDeep StasDeep commented Sep 3, 2018

@kmike could you please review the PR?

@kmike kmike changed the title Add ability to use FormRequest in contracts [MRG+1] Add ability to use FormRequest in contracts Sep 4, 2018
@kmike
Copy link
Member

@kmike kmike commented Sep 4, 2018

Looks good, thanks @StasDeep!

@kmike kmike added this to the v1.6 milestone Sep 5, 2018
@dangra dangra merged commit ae8a0dc into scrapy:master Sep 5, 2018
3 checks passed
3 checks passed
@codecov
codecov/patch 100% of diff hit (target 84.46%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants