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] Fix for #3034, CSV export unnecessary blank lines problem on Windows #3039

Merged
merged 1 commit into from Jul 3, 2018

Conversation

@ReLLL
Copy link
Contributor

@ReLLL ReLLL commented Dec 12, 2017

Fixed the issue I've mentioned there, this is the pull request to merge, (added one line), hope all is fine.
#3034

Closes #3034

@codecov
Copy link

@codecov codecov bot commented Dec 12, 2017

Codecov Report

Merging #3039 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3039   +/-   ##
=======================================
  Coverage   84.51%   84.51%           
=======================================
  Files         164      164           
  Lines        9270     9270           
  Branches     1380     1380           
=======================================
  Hits         7835     7835           
  Misses       1177     1177           
  Partials      258      258
Impacted Files Coverage Δ
scrapy/exporters.py 100% <ø> (ø) ⬆️
@cathalgarvey
Copy link
Contributor

@cathalgarvey cathalgarvey commented Feb 16, 2018

Hi @ReLLL, I'm not familiar with the CSV-on-Windows issue, but are we sure that this change will not affect output behaviour/quality on other platforms? If that's a sure then, then the change looks easy to merge, and I'm willing to give it a +1.

@stranac
Copy link

@stranac stranac commented Mar 8, 2018

@cathalgarvey There is no reason this should create a problem on any platforms, passing newline='' is the correct thing to do here, as documented in https://docs.python.org/3/library/csv.html#csv.reader

@cathalgarvey
Copy link
Contributor

@cathalgarvey cathalgarvey commented Mar 27, 2018

Well, in that case consider me MRG+1 for this PR. :)

@cathalgarvey cathalgarvey changed the title Fix for #3034, CSV export unnecessary blank lines problem on Windows [MRG+1] Fix for #3034, CSV export unnecessary blank lines problem on Windows Mar 27, 2018
@kmike
Copy link
Member

@kmike kmike commented Mar 27, 2018

Hey,

Is it possible to have a test case for this fix?

@dangra
Copy link
Member

@dangra dangra commented Jul 3, 2018

In #3315 I am trying to setup AppVeyor to run tests suite under Windows environment, the bad news is that the testsuite is far from passing due to filepaths, newlines and temporal files issues; but the good news is that this PR fixes tests for CSV exporters related to newline under Windows.

@dangra dangra merged commit 6f5c39d into scrapy:master Jul 3, 2018
2 checks passed
2 checks passed
@codecov
codecov/patch Coverage not affected when comparing a21b800...04914bf
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dangra
Copy link
Member

@dangra dangra commented Jul 3, 2018

Thanks @ReLLL!

If you feel like Scrapy support for Windows is important I'd like to encourage you to help with #3315.
I am not a Windows user and I don't have one at hand to run tests, waiting on appveyor feedback is very time consuming way to develop, any help is welcome.

@lilianaziolek
Copy link

@lilianaziolek lilianaziolek commented Jul 27, 2018

@dangra I'm using version 1.5.1 but despite seeing this in your master it doesn't seem to be in the latest released version. Is there any ETA on when this change will be available in a released version without having to hack my local build or build from source? Or am I missing a version that contains this change already?
Thanks!

@kmike kmike added this to the v1.6 milestone Jul 27, 2018
@kmike kmike mentioned this pull request Aug 15, 2018
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

6 participants