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] Add from_crawler constructor for feed exporters and storages #3348

Merged

Conversation

@elacuesta
Copy link
Member

@elacuesta elacuesta commented Jul 20, 2018

Picking up #1605 to add some changes requested by @kmike

storage = S3FeedStorage(uri)
access_key = os.environ.get('AWS_ACCESS_KEY_ID')
secret_key = os.environ.get('AWS_SECRET_ACCESS_KEY')
storage = S3FeedStorage(uri, access_key, secret_key)

This comment has been minimized.

@elacuesta

elacuesta Jul 20, 2018
Author Member

@kmike I believe this test is skipped, that's why passing access_key=Settings() didn't break. Is S3_TEST_FILE_URI available here?

This comment has been minimized.

@kmike

kmike Jul 20, 2018
Member

It is supposed to be set in the environment which runs tox - see

scrapy/tox.ini

Line 20 in 258121d

S3_TEST_FILE_URI

AFAIK we're not running s3 tests on travis if they require real s3 objects, i.e. this env variable is not set on Travis.

category=ScrapyDeprecationWarning,
stacklevel=2
)
access_key = settings['AWS_ACCESS_KEY_ID']

This comment has been minimized.

@elacuesta

elacuesta Jul 20, 2018
Author Member

Just checking: given that the operator here is an or, a KeyError might raise if one of the keys is missing, I guess that's expected? User code will break with a clear message I think.

This comment has been minimized.

@kmike

kmike Jul 20, 2018
Member

Settings object returns None if a key is missing; see

def __getitem__(self, opt_name):

This comment has been minimized.

@elacuesta

elacuesta Jul 20, 2018
Author Member

In that case, I think this might still break in the boto side if one of access_key/secret_key is not set either in the settings or in uri, the error message there will probably be clear enough.

@elacuesta elacuesta changed the title Enhancement/alternate feedexport constructors Add from_crawler constructor for feed exporters and storages Jul 20, 2018
@codecov
Copy link

@codecov codecov bot commented Jul 20, 2018

Codecov Report

Merging #3348 into master will increase coverage by 0.11%.
The diff coverage is 96.87%.

@@            Coverage Diff             @@
##           master    #3348      +/-   ##
==========================================
+ Coverage   84.23%   84.35%   +0.11%     
==========================================
  Files         167      167              
  Lines        9340     9359      +19     
  Branches     1384     1388       +4     
==========================================
+ Hits         7868     7895      +27     
+ Misses       1219     1209      -10     
- Partials      253      255       +2
Impacted Files Coverage Δ
scrapy/middleware.py 96% <100%> (-0.3%) ⬇️
scrapy/utils/misc.py 95.71% <100%> (+0.71%) ⬆️
scrapy/extensions/feedexport.py 78.46% <95%> (+5.5%) ⬆️
@kmike kmike changed the title Add from_crawler constructor for feed exporters and storages [MRG+1] Add from_crawler constructor for feed exporters and storages Jul 20, 2018
@kmike
Copy link
Member

@kmike kmike commented Jul 20, 2018

Looks good to me, thanks for picking it up @elacuesta, and thanks @jdemaeyer for the initial PR!

@kmike kmike added this to the v1.6 milestone Jul 20, 2018
@dangra dangra merged commit 732d7e1 into scrapy:master Jul 25, 2018
2 checks passed
2 checks passed
codecov/patch 96.87% of diff hit (target 84.23%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@elacuesta elacuesta deleted the elacuesta:enhancement/alternate-feedexport-constructors branch Jan 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants