-
Notifications
You must be signed in to change notification settings - Fork 79
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
support selector addition for form #20
base: master
Are you sure you want to change the base?
Conversation
eamonnmag
commented
May 5, 2017
- FIX Fixes add support for form selector #19
* FIX Fixes scrapy#19
Codecov Report
@@ Coverage Diff @@
## master #20 +/- ##
==========================================
+ Coverage 75.34% 76.31% +0.97%
==========================================
Files 1 1
Lines 73 76 +3
Branches 18 19 +1
==========================================
+ Hits 55 58 +3
Misses 16 16
Partials 2 2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and useful, but please remove all print()
calls.
def fill_login_form(url, body, username, password): | ||
def fill_login_form(url, body, username, password, *args, **kwargs): | ||
|
||
selector = kwargs.get('selector', '//form') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for **kwargs
, extend function signature to:
def fill_login_form(url, body, username, password, selector='//form'):
url = args[0] | ||
r = requests.get(url) | ||
headers = { | ||
'User-Agent': 'Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1)', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks useful but do you mind submitting this change in a different PR. I think it needs some extra thoughts like setting it from an command line flag that defaults to "Mozilla/4.0..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dangra Any thoughts on merging this PR, I am using this library in one of my projects, it will be very helpful for me if this feature is part of it.