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

CSV serialize #260

Merged
merged 2 commits into from
Jul 26, 2019
Merged

CSV serialize #260

merged 2 commits into from
Jul 26, 2019

Conversation

vlad-bm
Copy link
Contributor

@vlad-bm vlad-bm commented Jul 15, 2019

Adding class for serializing records into CSV format.

invenio_records_rest/serializers/csv.py Outdated Show resolved Hide resolved
invenio_records_rest/config.py Outdated Show resolved Hide resolved
tests/test_serializer_csv.py Outdated Show resolved Hide resolved
invenio_records_rest/serializers/csv.py Outdated Show resolved Hide resolved
@ntarocco
Copy link
Contributor

Good job! 👍 Please have a look to Travis too that is failing!

invenio_records_rest/serializers/__init__.py Outdated Show resolved Hide resolved
invenio_records_rest/serializers/base.py Outdated Show resolved Hide resolved
invenio_records_rest/serializers/csv.py Outdated Show resolved Hide resolved
invenio_records_rest/serializers/csv.py Outdated Show resolved Hide resolved
@vlad-bm vlad-bm marked this pull request as ready for review July 18, 2019 09:05
@vlad-bm vlad-bm requested a review from ntarocco July 18, 2019 09:05
@vlad-bm vlad-bm requested a review from egabancho July 18, 2019 14:17
@ntarocco ntarocco requested review from slint and removed request for ntarocco July 18, 2019 20:37
@ntarocco
Copy link
Contributor

@vladbmihaescu @egabancho @slint optimized the code with in memory buffer for the CSV output and return a generator. It should be more efficient with large amount of results. Added tests.
I have tested with a REST client and it looks working fine.

Copy link
Member

@slint slint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the "implementation aesthetics" comment, LGTM 👍

headers.update(rec.keys())

# write the CSV output in memory
output = StringIO()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, I prefer the second solution from this SO thread (the one with the Line class), it looks a bit higher-level (i.e. no .seek and .truncate calls)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, this is much better! Will change that! 👍

def __init__(self, *args, **kwargs):
"""Initialize CSVSerializer.

:param csv_excluded_fields: list of complete paths of the fields that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must admit I find it more intuitive to specify what to include instead of what to exclude, but that's my personal opinion. I.e. either you include all if the user didn't specify anything, or you include just what the user specfied.

@lnielsen
Copy link
Member

lnielsen commented Jul 19, 2019

Please also add tests with unicode characters (this is usually how I break the Python csv module) and check the import into e.g. Excel or other spreadsheet to ensure it loads nicely by default.

@lnielsen
Copy link
Member

Btw, please also ensure you add it to the documentation:
https://github.com/inveniosoftware/invenio-records-rest/blob/master/docs/api.rst

@lnielsen lnielsen added this to In progress in Files bundle sprint (July 22 to Aug 2) via automation Jul 23, 2019
@ntarocco
Copy link
Contributor

Please also add tests with unicode characters (this is usually how I break the Python csv module) and check the import into e.g. Excel or other spreadsheet to ensure it loads nicely by default.

actually my fault about this, I asked him to do it previously, he did it and then I pushed again removing that by mistake :) @vladbmihaescu sorry!

@lnielsen lnielsen moved this from In progress to Blocked in Files bundle sprint (July 22 to Aug 2) Jul 24, 2019
@lnielsen lnielsen moved this from Blocked to Pending review in Files bundle sprint (July 22 to Aug 2) Jul 24, 2019
@diegodelemos diegodelemos moved this from Pending review to In progress in Files bundle sprint (July 22 to Aug 2) Jul 26, 2019
@vlad-bm vlad-bm moved this from In progress to Pending review in Files bundle sprint (July 22 to Aug 2) Jul 26, 2019
@diegodelemos diegodelemos moved this from Pending review to In progress in Files bundle sprint (July 22 to Aug 2) Jul 26, 2019
@vlad-bm vlad-bm moved this from In progress to Pending review in Files bundle sprint (July 22 to Aug 2) Jul 26, 2019
@diegodelemos diegodelemos merged commit 819a824 into inveniosoftware:master Jul 26, 2019
Files bundle sprint (July 22 to Aug 2) automation moved this from Pending review to Done Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants