search

NanoHttpd / nanohttpd

Tiny, easily embeddable HTTP server in Java.
http://nanohttpd.org
BSD 3-Clause "New" or "Revised" License
6.92k stars 1.69k forks source link

MimeTypes! #499

Closed wyatt-herkamp closed 5 years ago

wyatt-herkamp commented 6 years ago

Basic MimeTypes. Source for MimeTypes: https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types

wyatt-herkamp commented 5 years ago

When will we be seeing 3.0?

wyatt-herkamp commented 5 years ago

@NunoAlexandre suggestion was added. getValue() -> to getContentType() value -> contentType

LordFokas commented 5 years ago

@wherkamp if you can add SVG and we'll merge this. I'll have to rework most of what I have in my stash anyways so there's no point in keeping this stuff frozen.

wyatt-herkamp commented 5 years ago

@LordFokas https://github.com/NanoHttpd/nanohttpd/pull/499/files#diff-6dbbc038ff7d75dc3b7c644ef0831160R59 Already added

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-1.4%) to 83.749% when pulling 0f81b7bcf76a2cd06a0a928c1bf11f5dfb591f56 on wherkamp:master into b04a342a4bcd0ff44be65138acffc73a29381c72 on NanoHttpd:master.

NunoAlexandre commented 5 years ago

I have three remarks still:

  1. The current commit messages are not very constructive. They could be:

instead of

  1. There is no reason to have both getContentType() and toString()

  2. This branch adds this module but doesn't use it anywhere. What would be the added value then? Shouldn't it be plugged in?

LordFokas commented 5 years ago

@wherkamp you have to leave the default toString(), it's not the same thing as the content type. @NunoAlexandre something may be missing but the MimeType is indeed used.

NunoAlexandre commented 5 years ago

@LordFokas

something may be missing but the MimeType is indeed used.

Where exactly is it used? This commit only adds the MimeType module but it doesn't seem to be used anywhere.

LordFokas commented 5 years ago

@NunoAlexandre it is meant to be plugged in here: https://github.com/NanoHttpd/nanohttpd/blob/master/core/src/main/java/org/nanohttpd/protocols/http/response/Response.java#L132

And yes, as I suspected, things are missing, and some other semi-related things are horribly wrong because of course they are. Fixing this library is a job for masochists.

wyatt-herkamp commented 5 years ago

So what needs to get changed for this PR to be accepted?

LordFokas commented 5 years ago

Nothing, I just skipped a beat to give Nuno a chance to give some more input.

However, now that you mention it, I have one final request. Can you, on the response class, create another response method that uses your MimeType instead of a string? The implementation for now should simply redirect to the string version with the contentType from the mime object (it's a 1-liner :p )

Thanks! :) I promise (unless there's a mistake somewhere) I'll merge right after that ;)

crearo commented 5 years ago

@NunoAlexandre (my 2 cents) you could squash and merge if you're not so happy with these commit messages showing up in history. Plus, 5 commits with such small changes don't make sense.

NunoAlexandre commented 5 years ago

@LordFokas @crearo I am not a member of this project, my feedback is merely suggestive.

Now, those commits are bad, but it's up to the members of the project to reconsider the standard.

@crearo, these should just be squashed indeed!

LordFokas commented 5 years ago

@NunoAlexandre being "just some guy" with "merely suggestive" but good feedback is how I became a member in the first place 😉 And just because you're not a member that doesn't mean I don't want to hear what you have to say, of course, and I'll be even more willing once I see you commit good stuff and giving more good feedback. I don't exactly disagree with what you've said above (about the toString()) but I do want to keep things simple for the devs but also the users, and semantic / suggestive names are crucial (trust me, I've been mucking with this code for years!).

Not everything the community sends PRs for is good (damn I've fought some battles to keep some nasty crap from being pulled in the past) but I also have the responsibility to drive the community to do things and PR them, so as long as what's coming in is better than what we currently have it's already a great start. This PR for instance matches what I want for 3.0 so I'm glad to have it, even though not everything is perfect (and of course I won't forget to squash).

Small progress is better than no progress 😄

wyatt-herkamp commented 5 years ago

@LordFokas is this ok?

wyatt-herkamp commented 5 years ago

I think I fixed it. Some how?? I tried to remove the 50 commits. But I messed up in my second commit message. I hope that doesn't interfere with the PR, I am sorry. If it does I can try to fix it.

NunoAlexandre commented 5 years ago

but I do want to keep things simple for the devs but also the users, and semantic/suggestive names are crucial

Right, but you better reconsider what you consider simple and good naming. We are talking about the MimeType type here. If I read mimeType.toString(), it's simple and clear, while mimeType.getContentType() isn't. What's the content type of a mime type? It's mixing things up: https://stackoverflow.com/questions/48691415/content-type-vs-mime-type.

NunoAlexandre commented 5 years ago

@wherkamp I don't think so.

  1. MimeTypes should be MimeType (singular)
  2. The private String contentType attribute better be mimetype or value as it was before.
  3. Drop the getContentType() and bring the toString back (see discussion above)
  4. Added evgk/patch-1 tells us nothing of what the commit does. Could you rename it to something like Integrate MimeType

You can git rebase -i origin/master to amend your commits title and changes in this branch.

wyatt-herkamp commented 5 years ago

I am done. Here use it if you want I don't care. This is too much work for one stupid file. https://gist.github.com/wherkamp/96f436833789cf4c1e6dd252017aa110

LordFokas commented 5 years ago

@NunoAlexandre this is what I meant by driving the community to do things. Just because you know how to code and use git doesn't mean you know how to handle what goes on on GitHub. And just because you wanted to nitpick every little detail that's one less user willing to get stuff done. Great job.

crearo commented 5 years ago

Ok in @NunoAlexandre 's defence, there was nothing wrong in what he said. I hate lousy commit messages, and enums typed in plural. Plus, having raised a couple issues before I know @LordFokas you prefer quality code. Better to prefer that than a larger community trotting bad code + history everywhere. That said, @NunoAlexandre : probably a good time to open a PR for this :D I've had to create a separate MimeType class because it wasn't already there when i was using nanohttpd.

LordFokas commented 5 years ago

If you've been here long enough then you know I can be ridiculously pedantic about things, but I'll have you know there's times I don't think it's appropriate. I'm not going to explain everything behind my logic with my acceptance here... also there's the fact I was ready to squash all that into a commit with a meaningful message.

Plus, you can easily educate users that PR bad code, and put aside users that refuse to be educated.

This being said, I think we should maybe draft a CONTIBUTING.md file to smooth out future PRs that might tend to go like this...

wyatt-herkamp commented 5 years ago

A. That class was a copy of some I used in my own projects. So I thought it might be nice inside of NanoHttpd. B. 3.0 is it going to be a rewrite?

LordFokas commented 5 years ago

3.0 is meant to be a refactoring. A massive one. So massive in fact that I have all the code I want in a VM somewhere and all the tests got fucked up and I ended up never commiting it.

But boy, does it have a good structure. I might have to redo all of it, step by step to get it to decent working condition.