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.

Sign up for GitHub

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

Jump to bottom

Fix xlsx file support #67

Merged
merged 6 commits into from Oct 26, 2020
Merged

Fix xlsx file support #67

merged 6 commits into from Oct 26, 2020

Conversation

quaxsze
Copy link
Contributor

@quaxsze quaxsze commented Oct 21, 2020 •
edited

fix #64

@quaxsze quaxsze requested a review from abulte October 21, 2020 11:51
@quaxsze
Copy link
Contributor Author

quaxsze commented Oct 21, 2020

The way we now check the file type might reject some files supported before. Do we need to support some type else than CSV, XLS and XLSX?

abulte
abulte reviewed Oct 22, 2020
View reviewed changes
Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

Add a test for xlsx (use an example file)

csvapi/parser.py Outdated Show resolved Hide resolved
csvapi/parser.py Outdated Show resolved Hide resolved
abulte
abulte reviewed Oct 22, 2020
View reviewed changes
csvapi/parser.py Outdated Show resolved Hide resolved
@quaxsze
Copy link
Contributor Author

quaxsze commented Oct 23, 2020

We already have a test for xls and xlsx files :

csvapi/tests/test_api.py

Line 328 in 7cb2993

@pytest.mark.parametrize('extension', ['xls', 'xlsx'])

I do not understand why it worked in the past. Is it because of the size of the file?

@quaxsze quaxsze requested a review from abulte October 23, 2020 09:31
abulte
abulte reviewed Oct 24, 2020
View reviewed changes
Copy link
Contributor

@abulte abulte left a comment

Choose a reason for hiding this comment

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

Better!

csvapi/parser.py Outdated Show resolved Hide resolved
abulte
abulte approved these changes Oct 26, 2020
View reviewed changes
csvapi/parser.py Outdated Show resolved Hide resolved
@abulte
Copy link
Contributor

abulte commented Oct 26, 2020

You can release and deploy after merge

@quaxsze quaxsze merged commit 7718b8b into master Oct 26, 2020
@quaxsze quaxsze deleted the fixXLSX branch October 26, 2020 10:11
@abulte abulte mentioned this pull request Nov 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abulte abulte abulte approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Specific XLS file eats up all the RAM
2 participants
@quaxsze @abulte