Skip to content

Conversation

@Megakuul
Copy link

@Megakuul Megakuul commented Mar 7, 2025

This is a initial proposal for #127

Copy link
Owner

@gabriel-vasile gabriel-vasile left a comment

Choose a reason for hiding this comment

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

@Megakuul thanks for the PR, looks good and solves the problem but there is one thing:
mime.Type() and mime.String() will return the same string value most of the times (only a few MIMEs actually have optional parameters.) Users might get confused and I don't think many will spend the time to read and understand the difference.
Also, mime.Type() encourages them to use == operator on string values of MIME types (the problem they we're discussing here: golang/go#31572 (comment))

if mime.Type() == types.Text { // this works correctly, but encourages the use of ==
  println("this is text")
}
if mime.String() == types.Text { // this is broken and some users might write it :(
  println("this is text")
}

I'd like to direct users more towards using safer functions: mime.Is() and EqualsAny().

So, the way I'm thinking to solve the problem in this PR:

  • add constant string values for each MIME (already done)
  • inside tree.go use the above constants (already done)
  • add a function to return all supported MIME string constants (not done)
    Not sure what to call this function. Maybe func All() []string?

.gitignore Outdated
@@ -0,0 +1,4 @@
# ignore emacs files
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this gitignore. I already made a decision to not add editor specific gitignore entries some time ago and it's only fair to the other people who tried to add this type of gitignore.

Copy link
Author

Choose a reason for hiding this comment

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

I do not really understand why this decision was made, but I see your point and removed the .gitignore.

Copy link
Owner

Choose a reason for hiding this comment

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

The reason was to avoid user environment specific entries, i.e. entries that are not related to the project itself.

When I made that decision, I used the golang/go as inspiration. golang/go@d647b61

@@ -0,0 +1,179 @@
package types
Copy link
Owner

Choose a reason for hiding this comment

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

I know some people have a types package and this package would conflict with it. Please move these constants in the root mimetype package.

Previously I suggested a new package, but I don't really know a good name for it. Seems mimetype is best location IMO.

Copy link
Author

Choose a reason for hiding this comment

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

@gabriel-vasile as you already pointed out in #127 I would rather not pollute the library scope with 150+ constants. Therefore I suggest keeping them in a separate package. The fact that types might collide is not a problem IMO, the package name should be clear and not unique as pointed out in effective go.

I used the name types because it's often used for similar packages (e.g. in aws-sdk-go-v2), but I am open to suggestions for other names, I just wouldn't put it in the root library scope...

Copy link
Owner

@gabriel-vasile gabriel-vasile Apr 5, 2025

Choose a reason for hiding this comment

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

aws-sdk is generated code. aws has an SDK specification and then they generate code for different programming languages. As it happens their generator cannot follow good-practices of all languages so they lean more towards java.

I'm still leaning towards using the root package for these reasons:

  • types is a popular name and I don't want to steal it from the user
  • mimetype seems good package to contain all mime types according to https://go.dev/blog/package-names

Edit: I checked how the docs would look when using the root package and seems ok to me...
The only downside I see is users will receive 100+ autocomplete suggestions when typing mimetype..

@Megakuul
Copy link
Author

@gabriel-vasile regarding the function that returns all constants: I've already added a function called SupportedMIMEs() that should return the flattened tree as proposed in #452.

I see your point that the user may get confused about Type() and String(). The problem is that .Is() and EqualsAny() are just for simple comparison. One of the main benefits of the types is that they are switchable, like so:

switch mime.Type() {
  case types.PLAIN:
    // handle text/plain
  case types.HTML:
    // handle text/html
  default:
    // handle default
}

This requires direct access to the type. To avoid this confusion, I wrapped the types into a type TYPE string which makes it clear to the user that he is accessing a specific datatype that holds a certain mimetype and not just an arbitrary string.

IMO, this change eliminates ambiguity for the user, as it requires an explicit conversion to a string (string(mime.Type())) to obtain the raw value.

@gabriel-vasile
Copy link
Owner

gabriel-vasile commented Apr 5, 2025

This requires direct access to the type. To avoid this confusion, I wrapped the types into a type TYPE string which makes it clear to the user that he is accessing a specific datatype that holds a certain mimetype and not just an arbitrary string.

IMO, this change eliminates ambiguity for the user, as it requires an explicit conversion to a string (string(mime.Type())) to obtain the raw value.

But this introduces a new type and type conversions...
I agree having the type would be awesome, but the existing code works with strings and I'd rather continue that way.
Go authors themselves had the same conversation golang/go#31572 (comment).

Edit:
About the switch argument, another option is a switch with no arguments:

m := mimetype.Detect(file)
switch {
    case m.Is(mimetype.SVG), m.Is(mimetype.PNG):
        // do things
    default:
        //
}

@Megakuul
Copy link
Author

Megakuul commented May 11, 2025

I don't really see the problem with a new type or a type conversion... I mean, .String() still returns a string and .Type() is new, so it's not breaking anyones code. Furthermore, since the conversion is a raw string "cast", there is virtually no conversion overhead.

I understand the go authors perspective, but they have a different context. They are building a standard library, and their main argument is not to overload it with specifics. This library is more specific, and that is perfectly fine IMO.

The switch argument is not just about the switch-statement. I may want to use the type as a map key or hash it (e.g. to use it as part of a cache key). The argument is more about having a processable value per mime and not just a pointer to a struct.

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.

2 participants