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 PythonItemExporter and CSVExporter for non-string item types #1737

Merged
merged 3 commits into from Jan 27, 2016

Conversation

@stummjr
Copy link
Member

@stummjr stummjr commented Jan 27, 2016

This PR fixes the error related to PythonItemExporter with non-string types reported here: c76190d#commitcomment-15693798.

It also fixes how CSVItemExporter represents non-string datatypes in CSV.

Instead of generating this (for a datetime object):
2,True,False,2015-01-01 01:01:01

It was generating this:
True,2,False,"datetime.datetime(2015, 1, 1, 1, 1, 1)"

@redapple redapple added this to the v1.1 milestone Jan 27, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Jan 27, 2016

Current coverage is 83.30%

Merging #1737 into master will increase coverage by +0.01% as of 075bb38

Powered by Codecov. Updated on successful CI builds.

@@ -200,7 +200,7 @@ def _build_row(self, values):
try:
yield to_native_str(s)
except TypeError:
yield to_native_str(repr(s))
yield to_native_str(str(s))

This comment has been minimized.

@eliasdorneles

eliasdorneles Jan 27, 2016
Member

This was introduced in one of my latest commits to PY3 exporters, I think using str is indeed a better fallback behavior than repr for CSV.

This replicates better the behavior of csv.writer.writerow, which is what I originally intended:

>>> import csv, io, datetime
>>> b = io.BytesIO()
>>> w = csv.writer(b)
>>> w.writerow([datetime.datetime.now()])
28L
>>> b.getvalue()
'2016-01-27 16:01:04.673800\r\n'

Now that I think of it, I wonder what would happen if we just yield s on TypeError and leave off the to_native_str.

This comment has been minimized.

@kmike

kmike Jan 27, 2016
Member

a small note: if there is a str(s) call there is no need for to_native_str, the result will always be the same

This comment has been minimized.

@stummjr

stummjr Jan 27, 2016
Author Member

Thanks, @eliasdorneles and @kmike. I just updated the PR.

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Jan 27, 2016

LGTM

@eliasdorneles eliasdorneles changed the title Fix PythonItemExporter and CSVExporter for non-string item types [MRG+1] Fix PythonItemExporter and CSVExporter for non-string item types Jan 27, 2016
self.assertExportResult(
item=item,
include_headers_line=False,
expected='22,False,3.14,2015-01-01 01:01:01\r\n'

This comment has been minimized.

@kmike

kmike Jan 27, 2016
Member

sorry for a stupid question, but where is the order of fields defined?

This comment has been minimized.

@eliasdorneles

eliasdorneles Jan 27, 2016
Member

it's not a stupid question, the order is no defined :)

the assertCsvEqual method does a crude comparison and calls sorted

This comment has been minimized.

@kmike

kmike Jan 27, 2016
Member

got it, thanks!

@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

Is fixing other exporters required?

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Jan 27, 2016

@kmike for this case, yes, XmlItemExporter also has a bug for this case but it's there also in Scrapy 1.0.4 (i.e., not a regression), @stummjr filed a issue: #1738

kmike added a commit that referenced this pull request Jan 27, 2016
[MRG+1] Fix PythonItemExporter and CSVExporter for non-string item types
@kmike kmike merged commit 394b991 into scrapy:master Jan 27, 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
@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

What about JSON? It'd be nice to have tests for all exporters :)

@kmike
Copy link
Member

@kmike kmike commented Jan 27, 2016

Thanks @stummjr!

@stummjr
Copy link
Member Author

@stummjr stummjr commented Jan 27, 2016

hey, @kmike ! I can add the tests for the other exporters too.
(edit: working on it)

@stummjr
Copy link
Member Author

@stummjr stummjr commented Jan 28, 2016

hey, @kmike!
I just included tests for the other exporters: #1742

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

5 participants
You can’t perform that action at this time.