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

Adds CSV MIME type that compatable with Windows #105

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

@@ -164,7 +164,7 @@ def prepare_for_identifier(regexps: Union[str, List[str]],
if isinstance(regexps, str):
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