Return to Revisions

I'll start off by saying this is actually really good. There are a few minor PEP8 errors:

• In 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",
                 callback=self.action)
  
   yield Request(
       url=baseurl + page + "?id=1",
       callback=self.action)
  

• Variables and functions should be named in snake_case. pagelist should be page_list, pageurl page_url, etc.

  Whilst not being a PEP8 error you may want to change page_list to say pages. 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, 'UserNameHere', 'PasswordHere', 'http://www.example.com'. I may even say that pagelist should be moved out too.

Now for comments on your business logic, as parse and 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 selector. This also contains the shortcut xpath which is equivalent to selector.xpath. 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 Parsel. The documentation on extract_first was also kinda lacking, so I read the source code again, and it seems like they always return strings. So you probably don't need the str around extract_first either.

• You don't define pagename unless page is not None. 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 return.

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']

class ShareSpider(scrapy.Spider):
    name = "sharespider"
    start_urls = [BASE_URL + '/public/login.aspx']

    def parse(self, response):
        yield scrapy.FormRequest.from_response(
            response,
            formxpath='//form[@id="login"]',
            formdata={
                'UserName': USER_NAME,             
                'Password': PASSWORD,             
                'Action':'1',
            },
            callback=self.after_login)

    def after_login(self, response):
        base_url = BASE_URL + '/public/'
        for page in PAGES:
            yield Request(
                url=base_url + page + "?id=1",
                callback=self.action)

    def action(self, response):
        page = re.search('public/(.*)id=1', response.url)
        if page:
            page_name = page.group(1)
            title = response.xpath('//title/text()').extract_first().strip()
            item = PageItem()
            item['pagename'] = page_name
            item['description'] = title
            yield item