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+2] added BLOCKING_FEED_STORAGE_PATH to settings #1847

Merged

Conversation

@aron-bordin
Copy link
Contributor

@aron-bordin aron-bordin commented Mar 5, 2016

PR Overview

I'm adding a new setting BLOCKING_FEED_STORAGE_PATH to configure a folder used to save temporary files before uploading them.

This setting defaults to None and will pick a system temporary folder. If a value is set, will raise an exception if it's not a valid directory and use it otherwise.

See #1779 for more details

@codecov-io
Copy link

@codecov-io codecov-io commented Mar 5, 2016

Current coverage is 83.19%

Merging #1847 into master will increase coverage by +0.01% as of 062b9f5

Powered by Codecov. Updated on successful CI builds.

if os.path.isdir(path):
folder = path
else:
raise NotADirectoryError(path)

This comment has been minimized.

@redapple

redapple Mar 17, 2016
Contributor

NotADirectoryError doesnt seem to be defined under Python 2

@redapple
Copy link
Contributor

@redapple redapple commented Mar 17, 2016

@aron-bordin , could you add tests for this feature and setting to scrapy.tests.feedexport?

@aron-bordin aron-bordin force-pushed the aron-bordin:add_blocking_storage_path_setting branch from f96ab1e to e5ed4b3 Mar 17, 2016
@aron-bordin
Copy link
Contributor Author

@aron-bordin aron-bordin commented Mar 17, 2016

Hi @redapple , I updated the PR. Please, take a look when possible.

@redapple
Copy link
Contributor

@redapple redapple commented Mar 18, 2016

@aron-bordin , could you add a test for the default case to make Codecov happy? :)

@@ -47,7 +47,16 @@ def store(file):
class BlockingFeedStorage(object):

def open(self, spider):
return TemporaryFile(prefix='feed-')
folder = None
path = spider.crawler.settings['BLOCKING_FEED_STORAGE_PATH']

This comment has been minimized.

@redapple

redapple Mar 18, 2016
Contributor

you can probably use path all along, and not use folder at all,
something like

        path = spider.crawler.settings.get('BLOCKING_FEED_STORAGE_PATH')
        if path:
            if not os.path.isdir(path):
                raise OSError('Not a Directory: ' + str(path))

        return NamedTemporaryFile(prefix='feed-', dir=path)
@aron-bordin aron-bordin force-pushed the aron-bordin:add_blocking_storage_path_setting branch 2 times, most recently from ce85442 to ac96a23 Mar 18, 2016
@aron-bordin
Copy link
Contributor Author

@aron-bordin aron-bordin commented Mar 18, 2016

@redapple updated

@aron-bordin aron-bordin force-pushed the aron-bordin:add_blocking_storage_path_setting branch from ac96a23 to 377f798 Mar 22, 2016
@redapple redapple changed the title added BLOCKING_FEED_STORAGE_PATH to settings [MRG+1] added BLOCKING_FEED_STORAGE_PATH to settings Mar 31, 2016
@redapple
Copy link
Contributor

@redapple redapple commented Mar 31, 2016

looks good to me. Thanks @aron-bordin !

@kmike
Copy link
Member

@kmike kmike commented Mar 31, 2016

The PR looks good 👍

But the BLOCKING_FEED_STORAGE_PATH name looks confusing, and other feed-related option names start with FEED_. What about FEED_STORAGE_TEMPDIR, or just FEED_TEMPDIR? Other ideas?

@aron-bordin aron-bordin force-pushed the aron-bordin:add_blocking_storage_path_setting branch from 377f798 to 80f0e12 Apr 1, 2016
@aron-bordin aron-bordin force-pushed the aron-bordin:add_blocking_storage_path_setting branch from 80f0e12 to 9250a5b Apr 1, 2016
@aron-bordin
Copy link
Contributor Author

@aron-bordin aron-bordin commented Apr 1, 2016

HI @kmike I updated the name to FEED_TEMPDIR, it sounds easier. Let me know if there is something else that I can change.

Thx

@kmike kmike changed the title [MRG+1] added BLOCKING_FEED_STORAGE_PATH to settings [MRG+2] added BLOCKING_FEED_STORAGE_PATH to settings Apr 1, 2016
@kmike
Copy link
Member

@kmike kmike commented Apr 1, 2016

@redapple are you OK with the updated option name?

@redapple
Copy link
Contributor

@redapple redapple commented Apr 1, 2016

Yup!

@redapple redapple merged commit bf7f675 into scrapy:master Apr 1, 2016
2 checks passed
2 checks passed
codecov/patch 100.00% of diff hit (target 100.00%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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