-
Notifications
You must be signed in to change notification settings - Fork 10.7k
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
Adding support for windows absolute pathlib.Path objects in FeedExporter #5939
Adding support for windows absolute pathlib.Path objects in FeedExporter #5939
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5939 +/- ##
=======================================
Coverage 88.88% 88.88%
=======================================
Files 162 162
Lines 11243 11244 +1
Branches 1826 1826
=======================================
+ Hits 9993 9994 +1
Misses 965 965
Partials 285 285
|
Nice, should the doc be updated though? |
I think both of those statements are still True. Because you still cannot use a string absolute path on windows. This only allows for using the absolute Edit: I did some experimenting and was unable to come up with a way to support a string absolute path on windows. However I did make changes in the documentation that clarifies that using the a Let me know if I should make any further changes. |
Thanks! |
This PR is related to #5739 as well as some conflicting information in the docs.
On the Feed Exports page, in the FEEDS subsection it mentions that you can use a URI or a
pathlib.Path
object as the target.scrapy/docs/topics/feed-exports.rst
Lines 412 to 423 in 33b418d
Earlier on the same page however under the Storage Backends section it mentions that you can use a standard string path as long as its an absolute path, but only on Unix systems.
scrapy/docs/topics/feed-exports.rst
Lines 149 to 161 in 33b418d
Both of these statements are accurate however, if one were to miss the second statement and only read the first you might think, as I did, that using an absolute
pathlib.Path
object on windows would work fine, when in fact it doesn't. For example using aPath(__file__).parent / "items.json"
fails when used on windows OS.My proposed solution to this, which is implemented in this PR is to simply get rid of the "only on Unix" caveat and make it work for windows as well. I achieved this by adding a conditional statement in the
FeedExporter.__init__
method that tests if the uri used in the settings is aPath
object, and if it is then to convert it to a filesystem uri.I also included an accompanying unit test that would fail prior to the changes.