pkg/mime: layout as mini-libraries

pkg/mime: layout as mini-libraries

Update version for publishing
Updated max unittest dependency version

R=ajohnsen@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36115

[Anders Johnsen, 2014-05-12 06:23:18]

Can you elaborate on these changes?

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/extension_m...
File pkg/mime/lib/src/extension_map.dart (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/extension_m...
pkg/mime/lib/src/extension_map.dart:8: const Map<String, String> EXTENSION_MAP =
const <String, String>{
Please keep the DEFAULT_

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/mime_type.dart
File pkg/mime/lib/src/mime_type.dart (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/mime_type.d...
pkg/mime/lib/src/mime_type.dart:15: int get defaultMagicNumbersMaxLength =>
DEFAULT_MAGIC_NUMBERS_MAX_LENGTH;
This is weird. Now we expose the same thing twice. Why?

https://codereview.chromium.org/272353002/diff/1/pkg/mime/pubspec.yaml
File pkg/mime/pubspec.yaml (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/pubspec.yaml#newcode2
pkg/mime/pubspec.yaml:2: version: 0.9.0+1
Push the version, as this is a breaking change (adding new public classes, etc)

[kevmoo, 2014-05-12 14:45:18]

PTAL

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/extension_m...
File pkg/mime/lib/src/extension_map.dart (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/extension_m...
pkg/mime/lib/src/extension_map.dart:8: const Map<String, String> EXTENSION_MAP =
const <String, String>{
On 2014/05/12 06:23:18, Anders Johnsen wrote:
> Please keep the DEFAULT_

Done, and renamed file to align

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/mime_type.dart
File pkg/mime/lib/src/mime_type.dart (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/lib/src/mime_type.d...
pkg/mime/lib/src/mime_type.dart:15: int get defaultMagicNumbersMaxLength =>
DEFAULT_MAGIC_NUMBERS_MAX_LENGTH;
On 2014/05/12 06:23:18, Anders Johnsen wrote:
> This is weird. Now we expose the same thing twice. Why?

Keep in mind the imported magic_number file is not exported. It's an
implementation detail.

I've rolled this into one const, though.

https://codereview.chromium.org/272353002/diff/1/pkg/mime/pubspec.yaml
File pkg/mime/pubspec.yaml (right):

https://codereview.chromium.org/272353002/diff/1/pkg/mime/pubspec.yaml#newcode2
pkg/mime/pubspec.yaml:2: version: 0.9.0+1
On 2014/05/12 06:23:18, Anders Johnsen wrote:
> Push the version, as this is a breaking change (adding new public classes,
etc)

Not a breaking change. The new "public" APIs are all in src and not exported.

The types the user sees when importing pkg:mime/mime.dart are unchanged.

[Anders Johnsen, 2014-05-13 09:16:22]

lgtm

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
File pkg/mime/lib/src/default_extension_map.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
pkg/mime/lib/src/default_extension_map.dart:8: const Map<String, String>
DEFAULT_EXTENSION_MAP = const <String, String>{
Please un-const this. Const maps are really slow for lookups.

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
File pkg/mime/lib/src/mime_type.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
pkg/mime/lib/src/mime_type.dart:16: const int defaultMagicNumbersMaxLength = 12;
Thinking about it, this should really not be public. Can you deprecate it?

[kevmoo, 2014-05-13 17:48:39]

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
File pkg/mime/lib/src/default_extension_map.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
pkg/mime/lib/src/default_extension_map.dart:8: const Map<String, String>
DEFAULT_EXTENSION_MAP = const <String, String>{
On 2014/05/13 09:16:22, Anders Johnsen wrote:
> Please un-const this. Const maps are really slow for lookups.

Are they different than a normal LinkedHashMap?

I can't imagine that this lookup would be anywhere close to critical performance
path for any user of this library.

Perhaps we should use a HashMap instead?

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
File pkg/mime/lib/src/mime_type.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
pkg/mime/lib/src/mime_type.dart:16: const int defaultMagicNumbersMaxLength = 12;
On 2014/05/13 09:16:22, Anders Johnsen wrote:
> Thinking about it, this should really not be public. Can you deprecate it?

Let's talk about how to expose this. Perhaps we can expose a const impl of the
global resolver instead?

I'm going to push this set of changes as-is and I'm happy to discuss evolution
moving forward.

[kevmoo, 2014-05-13 17:49:11]

Committed patchset #2 manually as r36115 (presubmit successful).

[Anders Johnsen, 2014-05-14 07:36:07]

Without these comments addressed (or at least discussed), it's not LGTM.

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
File pkg/mime/lib/src/default_extension_map.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
pkg/mime/lib/src/default_extension_map.dart:8: const Map<String, String>
DEFAULT_EXTENSION_MAP = const <String, String>{
On 2014/05/13 17:48:39, kevmoo wrote:
> On 2014/05/13 09:16:22, Anders Johnsen wrote:
> > Please un-const this. Const maps are really slow for lookups.
> 
> Are they different than a normal LinkedHashMap?
> 
> I can't imagine that this lookup would be anywhere close to critical
performance
> path for any user of this library.
> 
> Perhaps we should use a HashMap instead?

Please don't commit while there are comments.

Const-maps lookups are linear, so we compare against ~1000 strings to find a
single entry.

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
File pkg/mime/lib/src/mime_type.dart (right):

https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
pkg/mime/lib/src/mime_type.dart:16: const int defaultMagicNumbersMaxLength = 12;
On 2014/05/13 17:48:39, kevmoo wrote:
> On 2014/05/13 09:16:22, Anders Johnsen wrote:
> > Thinking about it, this should really not be public. Can you deprecate it?
> 
> Let's talk about how to expose this. Perhaps we can expose a const impl of the
> global resolver instead?
> 
> I'm going to push this set of changes as-is and I'm happy to discuss evolution
> moving forward.

But this entry is not useful for the user _at all_, as it's an implementation
detail! I would really have preferred to have this deprecated in the next push.
I don't think it's the right solution to jump the gun and push this, _before_ a
discussion can take place.

[kevmoo, 2014-05-14 15:49:28]

On 2014/05/14 07:36:07, Anders Johnsen wrote:
> Without these comments addressed (or at least discussed), it's not LGTM.
> 
>
https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
> File pkg/mime/lib/src/default_extension_map.dart (right):
> 
>
https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/default...
> pkg/mime/lib/src/default_extension_map.dart:8: const Map<String, String>
> DEFAULT_EXTENSION_MAP = const <String, String>{
> On 2014/05/13 17:48:39, kevmoo wrote:
> > On 2014/05/13 09:16:22, Anders Johnsen wrote:
> > > Please un-const this. Const maps are really slow for lookups.
> > 
> > Are they different than a normal LinkedHashMap?
> > 
> > I can't imagine that this lookup would be anywhere close to critical
> performance
> > path for any user of this library.
> > 
> > Perhaps we should use a HashMap instead?
> 
> Please don't commit while there are comments.
> 
> Const-maps lookups are linear, so we compare against ~1000 strings to find a
> single entry.
> 
>
https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
> File pkg/mime/lib/src/mime_type.dart (right):
> 
>
https://codereview.chromium.org/272353002/diff/20001/pkg/mime/lib/src/mime_ty...
> pkg/mime/lib/src/mime_type.dart:16: const int defaultMagicNumbersMaxLength =
12;
> On 2014/05/13 17:48:39, kevmoo wrote:
> > On 2014/05/13 09:16:22, Anders Johnsen wrote:
> > > Thinking about it, this should really not be public. Can you deprecate it?
> > 
> > Let's talk about how to expose this. Perhaps we can expose a const impl of
the
> > global resolver instead?
> > 
> > I'm going to push this set of changes as-is and I'm happy to discuss
evolution
> > moving forward.
> 
> But this entry is not useful for the user _at all_, as it's an implementation
> detail! I would really have preferred to have this deprecated in the next
push.
> I don't think it's the right solution to jump the gun and push this, _before_
a
> discussion can take place.

I'm sorry, Anders. I didn't read the importance of your suggestions.

Both were beyond the scope of the initial CL: the map has always been const,
etc.

The release I made was just a +1. I'm happy to review any further changes you'd
like to make. Publishing a +X release of a package is trivial.

Again, my apologies for plowing through your comments.

[Anders Johnsen, 2014-05-20 07:48:45]

Okay, let's work on cleaning up this package. I'll send you CL with my proposed
fixes, and we can take it from there! :)