I'd be more comfortable with this PR if it didn't include this API.

Why do we need this one?

hi @natebosch , thanks for having a look

this library does not know all possible file formats and I think it's neither realistic nor a goal of this lib to achieve that. The library also seems to be seeded with quite a random list of formats. So it can be quite a frequent situation in which a user of this library needs a format added. A pull request could be made, but the release cycle is not that fast and it's also a bit of overhead for maintainers.

That's why I think it may be useful to have this method so users don't immediately get stuck.

Similarly, this library already provides addExtension.

The 2nd usecase is one in which the user of the lib has a different preference than the one that is included. For example, if one prefers 'text/plain' to map to 'conf' as it currently does, one can still configure that old behavior.

Sorry for the slow response.

I'm worried about having an addMimeType that only impacts extensionFromMime and doesn't impact lookupMimeType.

We could either make this signature addMimeType(String mimeType, String extension, {List<int>? headerBytes}) and try to make this work with lookupMimeType, or we could consider an alternative name like `addExtensionPreference.

@devoncarew @lrhn WDYT?

Generally, I worry about global mutable state.
This is a kind of registry, where any code can add its own preferences, but they then also affect everybody else.

I'd prefer a structure where you have a Mime object instead of global state and functions. Then you can get your own object, modify it, and pass it around to your own code, without affecting other people's Mime objects.
That's a larger change, though.

I don't think we'll be able to do any refactoring of existing usage.

We could add a MimeRegistry class which can be overridden like this. The top-level APIs would continue to use the global map, the MimeRegistry would fall back on the global map, but it could also store local overrides. @lrhn WDYT about something like this?

@lrhn - Should I work on a PR with an implementation of a MimeRegistry, or do you think we can get by with the top level approach?

We can definitely "get by" with the top-level approach, but it's inherently fragile. I prefer to avoid adding fragile features.

What I'd do, if I had all the time in the world, is to have an immutable registry, and a mutator "builder" type.
The top-level functions should then just forward to the corresponding functions on MimeRegistry.defaultRegistry.

abstract interface class MimeRegistry {
  static get MimeRegistry currentRegistry => ... (probably something that can be zone overridden) ...;

  factory MimeRegistry.empty() = ...;

  int get defaultMagicNumbersMaxLength;
  String lookup(String path, {List<int>? headerBytes});
  String extensionFor(String mimeType);

  MimeRegistryBuilder get build;
  MimeRegistry rebuild(void Function(MimeRegistryBuilder) mutator);
}

abstract interface class MimeRegistryBuilder {
  // I don't know what you'd want, but something like:

  /// Adds mime type, if it doesn't already exist. 
  /// Adds [extension] to the extensions for [mimeType].
  /// Set the header-recognizer function to [headerRecognizer].
  void addMimeType(String mimeType, {String? extension, bool Function(List<int>) headerRecognizer});
  void removeMimeType(String mimeType);
  void addExtension(String mimeType, String additionalExtension);
  void removeExtension(String mimeType, String extension);
  // Maybe some inspection functions, but it's a little weird if those are not available on the registry itself.
  Iterable<String> get allMimeTypes;
  Iterable<String> allExtensionsFor(String mimeType);
  bool Function(List<int>)? headerRecognizerFor(String mimeType);
}

The builder is building on top of an existing registry, and it can either defer to it, lazily copy from it, or eagerly clone.
I'd make the actual implementations hidden.

But I might just be over-engineering. Always a risk 😉

What I'd do, if I had all the time in the world

What do you think we should do given that we have less time than that?

The same thing! The worry about lack of time for something else afterwards 😁

But the concrete problem we're trying to solve here seems to be solved without the addMimeType, so I'd remove that, not add configurability in this PR, and just give access to the default file-type extensions.

Then, if we still need configurability, we can come back and look at it fresh, and not stall this PR.

removed addMimeType

I wouldn't have the "OrNull" on the name here. That's just repeating the type.
It distinguishes it from the function below, but see comment there.

done

The name is a noun phrase, so treat this as a parameterized getter. That includes documenting it using a noun phrase too.
(At least the first sentence, after that it's OK to talk about returning a value.)

/// File extension for a given MIME type, if available.
///
/// Returns `null` if [mimeType] has no associated extension.
/// If [mimeType] has multiple associated extensions,
/// the preferred extension is returned.

Don't mention the map, it's not part of the public API, so a reader won't know what "the map" is.

done (had to look up what a 'noun phrase' is, so not sure if I did right ;) )

Needs documentation.
But I'd just drop this method. The user can easily do .extensionFromMime(myMime) ?? orElse() themselves.
Or .extensionFromMime(myMime)! if they know it's there.

done

btw, I now remember I did this to preserve backward compatibility..

I'd just inline that extension method here:

  for (var entry in defaultExtensionsMap.entries) {
     if (entry.value == mimeTypeLC) return entry.key;  
  }
  return null;

It's far too much (unnecessary) syntactic overhead to add your own firstWhereOrNull extension (takes up nine lines further down) to save, basically, one line of code here. And I personally don't find it easier to read either.

done

(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.)

done

(Avoid abbreviations, so LC could be lowerCase, but I'd probably just do:

 mimeType = mimeType.toLowerCase();

and not rename.)

I thought it would be bad practice to overwrite incoming params?
anyway, it turned out the function became so trivial I didn't need the var anymore

It's not possible for code to add to this map, right?
If so, it proably should be const.

indeed, not anymore ;)

but applying the optimization made it no longer possible

So the point of this code is that the defaultExtensionMap has multiple extensions mapping to the same mime type, so the reverse lookup can choose the "wrong" (not preferred) extension.

Could we create a new map by reversing the defaultExtensionMap and then overriding the ones that we care about, like:

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.

done