Skip to content

Allow spider attr#15

Open
pawelmhm wants to merge 7 commits intoscrapy-plugins:masterfrom
pawelmhm:allow_spider_attr
Open

Allow spider attr#15
pawelmhm wants to merge 7 commits intoscrapy-plugins:masterfrom
pawelmhm:allow_spider_attr

Conversation

@pawelmhm
Copy link

@pawelmhm pawelmhm commented Apr 10, 2015

This is about #14 and
#11

opening this for discussion, needs tests and improvements, would be cool to get some feedback if it's in good direction

@chekunkov chekunkov mentioned this pull request Apr 10, 2015
Open
chekunkov
chekunkov reviewed Apr 10, 2015
View reviewed changes
scrapyjs/middleware.py
Copy link

@chekunkov chekunkov Apr 10, 2015

Choose a reason for hiding this comment

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

@kmike are you okay with this change. What was usecase of .setdefault() here? Does it have any sense to create request with one url and specify another url in meta?

Copy link
Member

@kmike kmike Apr 10, 2015

Choose a reason for hiding this comment

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

Maybe I was thinking of allowing user not to use 'url' argument at all. This argument is not required for Splash scripts - e.g. you can render a chunk of HTML using splash:set_content. But the meta syntax doesn't make much sense in this case; something in lines of #12 could be a better fit.

pawelmhm added 2 commits May 26, 2015 09:10
@pawelmhm
[spider attr] removed html_response
2e7407d 
* removed process_html (render response in html feature) from this PR
* removed remote request tests with extra spider
@pawelmhm
[tests] refactors tests, adds tests for spider attr
bed8998 
* refactors tests from functions to objects inheriting
from unittest.TestCase
* adds tests for enabling middleware with spider attribute
@pawelmhm
Copy link
Author

pawelmhm commented May 26, 2015

updated after discussions

@chekunkov
Copy link

chekunkov commented Jun 9, 2015

@pawelmhm tests are failing

chekunkov
chekunkov reviewed Jun 9, 2015
View reviewed changes
scrapyjs/middleware.py Outdated
Copy link

@chekunkov chekunkov Jun 9, 2015

Choose a reason for hiding this comment

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

looks like unused import

Copy link
Author

@pawelmhm pawelmhm Jun 19, 2015

Choose a reason for hiding this comment

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

good point, this is fixed now

@pawelmhm
Copy link
Author

pawelmhm commented Jun 19, 2015

TODO

  • docs

something else?

@Gallaecio
Copy link
Contributor

Gallaecio commented Jul 11, 2019

I know it has been a rather long time, but it looks like this is still a valid pull request, and a nice once at that. It seems to me like it’s only missing renaming dont_proxy to something else and covering the changes in the documentation.

@pawelmhm Do you have plans to resume work on it at some point?

Gallaecio pushed a commit to Gallaecio/scrapy-splash that referenced this pull request Aug 12, 2019
@pawelmhm @Gallaecio
[scrapy-plugins#15/spider_attribute] adds proper tests
6a82190
@Gallaecio Gallaecio mentioned this pull request Aug 12, 2019
Open
@Gallaecio
Copy link
Contributor

Gallaecio commented Aug 12, 2019

Continued at #235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pawelmhm @chekunkov @Gallaecio @kmike