-
Notifications
You must be signed in to change notification settings - Fork 50
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
provide better defaults for mimetype to extension mapping #81
base: master
Are you sure you want to change the base?
Changes from 25 commits
86114bd
9f1ef55
c9e3e15
0f37a4b
f4c4a4f
a9ec842
02b9dcb
1ad29e9
84a8f43
1f2ab54
fbf8f68
6600a3f
6fe7ecb
d57b9fc
0d6c519
bb9fc15
7280615
40107ff
c384ef3
9ca2e5b
76f80f6
7b2804f
65c7016
e8a6ed5
f8642bd
8acfbe4
f20c40f
2be331f
60e9a7b
54da123
d190914
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'default_extension_map.dart'; | ||
|
||
/// Add an override for common extensions since different extensions may map | ||
/// to the same MIME type. | ||
final Map<String, String> _preferredExtensionsMap = <String, String>{ | ||
'application/vnd.ms-excel': 'xls', | ||
'application/vnd.ms-powerpoint': 'ppt', | ||
'image/jpeg': 'jpg', | ||
'image/tiff': 'tif', | ||
'image/svg+xml': 'svg', | ||
'text/calendar': 'ics', | ||
'text/javascript': 'js', | ||
'text/plain': 'txt', | ||
'text/sgml': 'sgml', | ||
'text/x-pascal': 'pas', | ||
'video/mp4': 'mp4', | ||
'video/mpeg': 'mpg', | ||
'video/quicktime': 'mov', | ||
'video/x-matroska': 'mkv', | ||
}; | ||
|
||
/// The extension for a given MIME type. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Consider changing "extension" to "file extension". Maybe it's just me having worked too much with "extension types" recently, but "extension" isn't feeling precise enough.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
/// | ||
/// If there are multiple extensions for [mimeType], return preferred extension | ||
/// if defined, otherwise an extension chosen by the library. | ||
/// If no extension is found, `null` is returned. | ||
String? extensionFromMime(String mimeType) { | ||
final mimeTypeLC = mimeType.toLowerCase(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (Avoid abbreviations, so mimeType = mimeType.toLowerCase(); and not rename.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought it would be bad practice to overwrite incoming params? |
||
if (_preferredExtensionsMap.containsKey(mimeTypeLC)) { | ||
return _preferredExtensionsMap[mimeTypeLC]!; | ||
} | ||
|
||
for (final entry in defaultExtensionMap.entries) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the point of this code is that the Could we create a new map by reversing the Map<String, String> _preferredExtensionsMap = {for (var entry in defaultExtensiosnMap.entries) entry.value: entry.key}..addAll({
'application/vnd.ms-excel': 'xls',
'application/vnd.ms-powerpoint': 'ppt',
'image/jpeg': 'jpg',
'image/tiff': 'tif',
'image/svg+xml': 'svg',
'text/calendar': 'ics',
'text/javascript': 'js',
'text/plain': 'txt',
'text/sgml': 'sgml',
'text/x-pascal': 'pas',
'video/mp4': 'mp4',
'video/mpeg': 'mpg',
'video/quicktime': 'mov',
'video/x-matroska': 'mkv',
}); Then we only need to look up in one map (and not do a linear search). That's a possible optimization, what we have is already better than what was before, so no change required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
if (entry.value == mimeTypeLC) return entry.key; | ||
} | ||
return null; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
// Copyright (c) 2023, the Dart project authors. Please see the AUTHORS file | ||
// for details. All rights reserved. Use of this source code is governed by a | ||
// BSD-style license that can be found in the LICENSE file. | ||
|
||
import 'package:mime/mime.dart'; | ||
import 'package:test/test.dart'; | ||
|
||
void main() { | ||
group('global-lookup-mime-type', () { | ||
test('valid-mime-type', () { | ||
expect(extensionFromMime('text/x-dart'), equals('dart')); | ||
expect(extensionFromMime('text/javascript'), equals('js')); | ||
expect(extensionFromMime('application/java-archive'), equals('jar')); | ||
expect(extensionFromMime('application/json'), equals('json')); | ||
expect(extensionFromMime('application/pdf'), equals('pdf')); | ||
expect(extensionFromMime('application/vnd.ms-excel'), equals('xls')); | ||
expect(extensionFromMime('application/xhtml+xml'), equals('xht')); | ||
expect(extensionFromMime('image/jpeg'), equals('jpg')); | ||
expect(extensionFromMime('image/png'), equals('png')); | ||
expect(extensionFromMime('text/css'), equals('css')); | ||
expect(extensionFromMime('text/html'), equals('htm')); | ||
expect(extensionFromMime('text/plain'), equals('txt')); | ||
expect(extensionFromMime('text/x-c'), equals('c')); | ||
}); | ||
|
||
test('invalid-mime-type', () { | ||
expect(extensionFromMime('invalid-mime-type'), isNull); | ||
expect(extensionFromMime('invalid/mime/type'), isNull); | ||
}); | ||
|
||
test('unknown-mime-type', () { | ||
expect(extensionFromMime('application/to-be-invented'), isNull); | ||
}); | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not possible for code to add to this map, right?
If so, it proably should be
const
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indeed, not anymore ;)
but applying the optimization made it no longer possible