I'll start off by saying this is actually really good.
There are a few minor PEP8 errors:
after_login you call
Request. the argument
url shouldn't have a space on the right side of the equals.
Another thing in
Request. The argument
callback needs at least one more indent.
However you should either aligned with opening delimiter or use hanging indents.
And so you should change this to either of:
yield Request(url=baseurl + page + "?id=1",
url=baseurl + page + "?id=1",
Variables and functions should be named in
pagelist should be
Whilst not being a PEP8 error you may want to change
page_list to say
This is as multiple pages are probably a list, and sounds more fluid.
I say the above as consistency is the best way to allow others to easily read your work.
But they are quite minor.
Personally I'd move the
baseurl out of the class and make it a constant.
This allows you to easily change the url, say the company you're scraping change their company/domain name or their top-level domain.
I'd also move other system settings,
I may even say that
pagelist should be moved out too.
Now for comments on your business logic, as
after_login are very short 3 line functions,
there's honestly not much to say about them.
action however has a few problems:
response.url is a string and so you shouldn't cast it to one.
This confused me as I it made me think
response.url would be something that isn't a string.
You seem to be getting the
response as an instance of
TextResponse, which contains
This also contains the shortcut
xpath which is equivalent to
Which allows you to reduce the size of that large command.
Unfortunately scrapy doesn't document
extract_first, but looking at the source they use
The documentation on
extract_first was also kinda lacking, so I read the source code again, and it seems like they always return strings or
So you probably don't need the
extract_first if you set default to an empty string.
You don't define
page is not
But you go on to use it regardless.
This is wrong, and can lead to errors.
There are two ways to come at this, either silently fail.
Or raise an exception, where
NameError: name 'pagename' is not defined is not a good error.
Finally it's strange to see
yield rather than
return, you're only going to ever return one thing.
And so unless it's a requirement imposed by scrapy I'd change it to
And so if I were to improve it, leaving
yield and failing silently, I'd end with:
BASE_URL = 'http://www.example.com'
USER_NAME = 'UserNameHere'
PASSWORD = 'PasswordHere'
PAGES = ['page1.aspx', 'page2.aspx', 'page3.aspx', 'page4.aspx']
name = "sharespider"
start_urls = [BASE_URL + '/public/login.aspx']
def parse(self, response):
def after_login(self, response):
base_url = BASE_URL + '/public/'
for page in PAGES:
url=base_url + page + "?id=1",
def action(self, response):
page = re.search('public/(.*)id=1', response.url)
page_name = page.group(1)
title = response.xpath('//title/text()').extract_first('').strip()
item = PageItem()
item['pagename'] = page_name
item['description'] = title