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] Implement FormRequest.from_response CSS support #1382

Merged
merged 1 commit into from Jan 21, 2016

Conversation

@barraponto
Copy link
Contributor

@barraponto barraponto commented Jul 24, 2015

Since we have a formxpath argument, it makes sense to have formcss argument.

Solves #1374

@kmike
Copy link
Member

@kmike kmike commented Jul 29, 2015

LGTM.

@kmike kmike changed the title Implement FormRequest.from_response CSS support [MRG+1] Implement FormRequest.from_response CSS support Jul 29, 2015
@barraponto
Copy link
Contributor Author

@barraponto barraponto commented Jul 30, 2015

There is also the case where both formxpath and formcss are defined: as I've implemented it, formcss overrides formxpath. Should we document it or should we raise some error?

@kmike
Copy link
Member

@kmike kmike commented Jul 30, 2015

There is also the case where both formxpath and formcss are defined: as I've implemented it, formcss overrides formxpath. Should we document it or should we raise some error?

Yes, documenting it could be nice. But for other arguments the priority is also neither documented nor validated. Your PR is not making things any worse and adds a feature which fits into current design, that's why I'm +1 to merge it :)

In the long term we need to cleanup FormRequest. Maybe it should accept a Selector in addition to Response as a first argument? Or an lxml <form> element directly. This way we won't have issues with xpath/css priorities.

These form... arguments are a mess. See e.g. #1137 - currently you can pass formname and get a form with a different name submitted.

@barraponto
Copy link
Contributor Author

@barraponto barraponto commented Jul 31, 2015

okay, then I'm fine with it :shipit:

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Dec 4, 2015

hey @barraponto, this looks good!
Could you solve the conflicts, so we can merge this?
Thanks, man!

@barraponto
Copy link
Contributor Author

@barraponto barraponto commented Jan 2, 2016

did a small refactor, leveraging parsel and moving the css-to-xpath conversion to a higher level (making for less internal changes).

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Jan 4, 2016

Very nice, thanks, @barraponto !
Can you change the test to use bytes (e.g. self.assertEqual(fs[b'one'], [b'1'])), so that it passes for PY33 too? Will be ready to merge after that. :)

@codecov-io
Copy link

@codecov-io codecov-io commented Jan 21, 2016

Current coverage is 82.95%

Merging #1382 into master will increase coverage by +0.03% as of 13fe6ea

Powered by Codecov. Updated on successful CI builds.

@barraponto
Copy link
Contributor Author

@barraponto barraponto commented Jan 21, 2016

Yay, got the green light from Travis!

eliasdorneles added a commit that referenced this issue Jan 21, 2016
[MRG+1] Implement FormRequest.from_response CSS support
@eliasdorneles eliasdorneles merged commit a6e5c84 into scrapy:master Jan 21, 2016
2 checks passed
@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Jan 21, 2016

Thanks a bunch @barraponto ! :)

@@ -339,6 +342,9 @@ fields with form data from :class:`Response` objects.
.. versionadded:: 0.17
The ``formxpath`` parameter.

.. versionadded:: 1.1.5
Copy link
Member

@kmike kmike Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have this version yet :) I think this change will be included in 1.1.0.

Copy link
Member

@eliasdorneles eliasdorneles Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry, I missed that (I shouldn't merge PRs after midnight!).
lemme fix that

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

5 participants