Skip to content

Conversation

@Gateswong
Copy link

@blais
Copy link
Member

blais commented Oct 1, 2022

So this mentions excel... not CSV. Why?

@hexhexD
Copy link

hexhexD commented Jul 1, 2023

I think having Excel as the default app for CSV results a MIME of application/vnd.ms-excel (It doesn't happen using other program as default app for CSV extension). I tested this patch and it can recognize CSV with or without Excel installation so I think it's a improvement over the original.

regexps = [regexps]
matchers = matchers or []
matchers.append(('mime', 'text/csv'))
matchers.append(('mime', '(text/csv)|(application/vnd\\.ms-excel)'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem with this change is that the importers based on beangulp.importers.csv will start matching all files with the Excel mime type, including xls ans xlsx files. This would be a backward compatibility break, and we should avoid this, especially for an importer base class that has been around for very very long.

What can be done is to add the mime matcher only if one has not been passed by the user already. This would allow you to add the matching of the application/vnd\\.ms-excel to your importer only. Something like this:

Suggested change
matchers.append(('mime', '(text/csv)|(application/vnd\\.ms-excel)'))
if not any(m[0] == 'mime' for m in matchers):
matchers.append(('mime', 'text/csv'))

if I remember correctly how this code is supposed to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants