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

Update form.py to improve existing capability #1137

Merged
merged 4 commits into from Jul 31, 2015
Merged

Conversation

@DharmeshPandav
Copy link
Contributor

@DharmeshPandav DharmeshPandav commented Apr 6, 2015

Add capability to search HTML Form using formid when using FormRequest.from_response()

refrenced issue :#1136

Add capability to search HTML Form using formid when using `FormRequest.from_response()`

refrenced issue :#1136
@pablohoffman
Copy link
Member

@pablohoffman pablohoffman commented Apr 9, 2015

Hi @DharmeshPandav, can you add a test?. Also, this seems to have broken the build - can you check Travis?

…ion to formname , formnumer before we go for xpath.. making it more idiomatic in nature
@DharmeshPandav
Copy link
Contributor Author

@DharmeshPandav DharmeshPandav commented Apr 12, 2015

HI @pablohoffman ..yes there was some issue with travis ..I have missed one variable in signature of function _get_form ...adding it and pushing the changes...travis is in progress

building test is not required..as test for this object is already defined

@dangra
Copy link
Member

@dangra dangra commented Apr 15, 2015

As per Mozilla docs id in HTML5 is the successor of name attribute from HTML4, so +1 to some way of shortcut for it.

@DharmeshPandav where is the test you mention? the function argument is new so it need a test case IMO.

…ml form election by formid attribute
@DharmeshPandav
Copy link
Contributor Author

@DharmeshPandav DharmeshPandav commented Apr 15, 2015

@dangra , @pablohoffman : Yes i agree 👍 , added four unit tests for this new form attribute support- formid

  1. test_from_response_formid_exists
  2. test_from_response_formid_notexists_fallback_formname
  3. test_from_response_formname_notexists_fallback_formid
  4. test_from_response_formid_notexist
  5. test_from_response_formid_errors_formnumber
r1 = self.request_class.from_response(response, formid="form3")
self.assertEqual(r1.method, 'POST')
fs = _qs(r1)
self.assertEqual(fs, {'one': ['1']})

This comment has been minimized.

@kmike

kmike Apr 15, 2015
Member

Why does it return the first form (with id='form1') when formid="form3" is requested?

This comment has been minimized.

@DharmeshPandav

DharmeshPandav Apr 15, 2015
Author Contributor

short answer will be because that is what this test is supposed to do

For in-depth , do check the code and signature below :

# from_response method
@classmethod
    def from_response(cls, response, formname=None, formid=None, formnumber=0, formdata=None,
                      clickdata=None, dont_click=False, formxpath=None, **kwargs):
        kwargs.setdefault('encoding', response.encoding)
        form = _get_form(response, formname, formid, formnumber, formxpath)
        formdata = _get_inputs(form, formdata, dont_click, clickdata, response)
        url = _get_form_url(form, kwargs.pop('url', None))
        method = kwargs.pop('method', form.method)
        return cls(url=url, method=method, formdata=formdata, **kwargs)

# inside _get_form method
def _get_form(response, formname, formid, formnumber, formxpath):
    """Find the form element """
if formnumber is not None:
        try:
            form = forms[formnumber]
        except IndexError:
            raise IndexError("Form number %d not found in %s" %
                             (formnumber, response))
        else:
            return form

as you can see in form.py by default from_response assigns the value of * 0 to formnumber*
so here selection of the form by formnumber is like last resolve , a fall-back mechanism to prevent the code from failing if scrapy could not find any form element using the passed parameter i.e. formname , formid and formxpath

now since the form element is present in object , it will select the first form element available ( index - 0) and return it and then will call the submit method of that form...

the fall back sequence will be

  • first check for formname ,
  • if not present go for formid
  • if not present go for formxpath
  • if not present go for formnumber
    • if we will pass formnumber it will select that number of form
    • if we don't pass any parameter mentioned above (or passed one's are not found) ,scrapy will select 1st form (0th index)

as for this unit test ...we are checking that the scrapy indeed return the first form when passed formid parameter does not exists else this test will fail

IMHO , as per your doubt ..yes it should throw an error when form with specified attribute is not found but here we are falling back to first form

whether or not we should have a fallback mechanism like this , is a question for debate

i think we should go for first form available when user don't pass any attribute but when she is specific about form attribute to look for and it is not found , code should throw an error rather than fallback

…test for pull request #1137 - addition of new shortcut for html form election by formid attribute
@DharmeshPandav
Copy link
Contributor Author

@DharmeshPandav DharmeshPandav commented Jul 29, 2015

Hi
what do i need to do to have this pull request accepted/close/( may be cancel)

I am new to unit test writing , so used old test to write new ones for this change...let me know how can i improve and i shall complete them

dangra added a commit that referenced this pull request Jul 31, 2015
Update form.py to improve existing capability
@dangra dangra merged commit 786f626 into scrapy:master Jul 31, 2015
1 check passed
1 check passed
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
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.