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

Added Pathlib.Path support: Issue #3731 #4074

Merged
merged 19 commits into from Nov 12, 2019
Merged

Added Pathlib.Path support: Issue #3731 #4074

merged 19 commits into from Nov 12, 2019

Conversation

purvaudai
Copy link
Contributor

@purvaudai purvaudai commented Oct 13, 2019

Fixes #3731

@codecov
Copy link

@codecov codecov bot commented Oct 13, 2019

Codecov Report

Merging #4074 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4074      +/-   ##
==========================================
+ Coverage   83.36%   83.39%   +0.03%     
==========================================
  Files         165      165              
  Lines        9802     9803       +1     
  Branches     1462     1463       +1     
==========================================
+ Hits         8171     8175       +4     
+ Misses       1366     1365       -1     
+ Partials      265      263       -2
Impacted Files Coverage Δ
scrapy/extensions/feedexport.py 81.9% <100%> (ø) ⬆️
scrapy/utils/datatypes.py 55.72% <0%> (+1.27%) ⬆️
scrapy/utils/trackref.py 86.48% <0%> (+2.7%) ⬆️

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 14, 2019

I believe the proposed fix #3753 is slightly cleaner.

Also, both patches are missing an accompanying test that covers the change. Could you add one?

@purvaudai
Copy link
Contributor Author

@purvaudai purvaudai commented Oct 14, 2019

I am confused on how and where to add a test. Could you please guide a bit?

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 14, 2019

Have a look at the tests in https://github.com/scrapy/scrapy/blob/master/tests/test_feedexport.py, see if you can get some ideas. Otherwise, I’ll see if I can take a closer look myself and give you some better feedback.

@purvaudai
Copy link
Contributor Author

@purvaudai purvaudai commented Oct 14, 2019

I did look into that file. However, I could not even find the class which I had made changes to (FeedExporter) being imported into the test file. Also, I could not find any existing tests for FeedExporter from which I could get any ideas.

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 15, 2019

Forget about the class, navigating the internals of Scrapy can be daunting at first. Instead, focus on the way users would take advantage of your changes: the FEED_URI setting. You just need to test that setting FEED_URI to a Path works the same as using a string.

Existing tests get a temporary folder automatically created for them and assigned to FEED_URI. You can create a test that uses a custom FEED_URI value instead.

For example, based on test_export_no_items_store_empty and the temporary folder creation I’ve just mentioned, you could write something in the lines of:

@defer.inlineCallbacks
def test_pathlib_uri(self):
    tmpdir = tempfile.mkdtemp()
    feed_uri = Path(tmpdir) / 'res'
    settings = {
        'FEED_FORMAT': 'csv',
        'FEED_STORE_EMPTY': True,
        'FEED_URI': feed_uri,
    }
    data = yield self.exported_no_data(settings)
    self.assertEqual(data, b'')
    shutil.rmtree(tmpdir, ignore_errors=True)

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Oct 22, 2019

I think it’s OK not to support Python 2, as we will drop support soon, right after releasing the upcoming 1.8 version of Scrapy.

@purvaudai
Copy link
Contributor Author

@purvaudai purvaudai commented Nov 11, 2019

@Gallaecio I do not understand why the Travis CI build is not triggering for the last 2 commits. Am I doing anything wrong?

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Nov 11, 2019

Your changes are currently in conflict with the master branch. Until you resolve them, the tests cannot run.

@purvaudai
Copy link
Contributor Author

@purvaudai purvaudai commented Nov 11, 2019

@Gallaecio
Oh, I see. Now that the Travis-CI build has passed, what else is required now?

@Gallaecio
Copy link
Member

@Gallaecio Gallaecio commented Nov 11, 2019

Now people will review your changes. We usually merge a pull request once two core developers have approved it.

requirements-py3.txt Outdated Show resolved Hide resolved
tests/test_feedexport.py Outdated Show resolved Hide resolved
tests/test_feedexport.py Show resolved Hide resolved
wRAR
wRAR approved these changes Nov 12, 2019
@wRAR wRAR merged commit c913905 into scrapy:master Nov 12, 2019
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants