Skip to content
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

Added a new method to get all extensions. #26

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

thihara
Copy link

@thihara thihara commented Oct 15, 2016

Added a new method called 'allExtensions' to get all the extensions matching a given mime-type.

Added a new method called 'allExtensions' to get all the extensions matching a given mime-type.
Added information about how to use the allExtensions method.
Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Thank you for contributing, @thihara ! It looks like a few things:

  1. Can you include a little bit of docs in the PR?
  2. Can you add some tests to our test suite to cover the new functionality?
  3. If you can run npm run lint to lint the code and fix the linting issues, that would help a lot :) We use the Standard JS style (http://standardjs.com/) which, namely, requires no semicolons.

Thank you so much for helping out!

@thihara
Copy link
Author

thihara commented Oct 15, 2016

@dougwilson Done. There was an issue with the eslint command, it was ignoring the index.js file. Please resolve the conflict and merge.

README.md Outdated Show resolved Hide resolved
@dougwilson
Copy link
Contributor

Awesome, thanks, @thihara ! The GitHub web interface does not provide any method to resolve conflicts, so I will not be able to merge until I'm at a computer (vs being on my phone), so it may not be until tomorrow at the earliest that I can merge with a conflict :) Also, I noticed a typo in your docs. If you can fix that, that would be awesome, otherwise I'll get that resolved tomorrow or whenever I'm at a computer again for ya :)

@thihara
Copy link
Author

thihara commented Oct 15, 2016

@dougwilson I think you meant the missing 's' at the allExtension. I fixed that. Thanks. I'm not sure if I can resolve the conflict myself without write access.

@dougwilson
Copy link
Contributor

So since I don't have much to do on my phone, I was reading back up on this issue to see why it wasn't gotten to yet, and now I remember: this is because the current array of extensions is actually completely useless save for the first element in the array. Going back to koajs/koa#617 (comment) remined me on what the blocker to adding a real API to this module was, which was to actually get that array to be in a meaningful order for the use-cases that wanted to use the functionality that I was aware of.

What is the use-case you are trying to use this function for? Is it similar to that Koa one, or something different?

@dougwilson
Copy link
Contributor

I'm not sure if I can resolve the conflict myself without write access.

To resolve the conflict from your side you either need to simply remove the conflicting change from your PR (which is what I was going to do anyway--the package.json change on master is the one I will be keeping over the change here) or you can rebase your branch on top of the current master, which would force you to go through the resolve process and then force push an update to your branch.

Anyway, I can resolve the merge, but will just have to wait until I'm at a computer at the earliest, since the GitHub web interface provides no method to resolve conflicts (the merge button on this PR is just literally disabled with no action I can take to merge or resolve).

But in addition, I was reading back over the original issues, which brought up a question I posted in the previous comment if you want to discuss while I'm around right now :)

@dougwilson
Copy link
Contributor

I think you meant the missing 's' at the allExtension. I fixed that. Thanks.

haha, I didn't even notice that. What I saw was the example code had the function name of just "extension" rather than "allExtension". It's hard to review anything from a phone, I guess :) Small screen and scrolling through source code is not that great of an experience, haha.

@thihara
Copy link
Author

thihara commented Oct 15, 2016

@dougwilson Perhaps, but what I wanted to get was a list of possible extensions for a mime-type. This is just for some validations. Nothing like the issue you mentioned in koa.

Ah I think I'll let you do the merge in that case 😆

@dougwilson
Copy link
Contributor

Cool, will do :) So I guess I have nothing else to do what research around this issue :) One thing this module does is provide compatible APIs with the mime npm module. I then wondered: has that module added a similar API? It turns out no, but there is a PR to add this exact thing to that module: broofa/mime#136

We should probably coordinate to make sure that we both end up landing a function with the same name. If you want to take point on that, that'd be awesome! Otherwise, I'll go poke on that PR to see if we can get alignment on the function name prior to landing this.

@thihara
Copy link
Author

thihara commented Oct 15, 2016

Good luck with that. The PR in question seem to be open for months without a response. 💃
I'd consider that a dead repo for all intents and purposes.

mime-types is reverse compatible for all existing methods of node-mime and since no activity seem to be happening in that repo, I see no point in trying to be completely inter-operable. Specially for a function that doesn't exist in there yet.

Reverted lint command changes. Better one is available now in master branch.
@thihara
Copy link
Author

thihara commented Oct 16, 2016

@dougwilson I managed to remove the changes in package.json, since you said you will be discarding them anyway. Looks like we have a PR without conflicts now!

@thihara
Copy link
Author

thihara commented Oct 18, 2016

@dougwilson Any chance of approving this pull request soon?

@dougwilson
Copy link
Contributor

Sorry, I got busy this weekend. Let me just sync up with that other PR real quick to get alignment on the name. I meant to do that Sunday, but did not get to it. I'll provide an update soon with what I find out.

@thihara
Copy link
Author

thihara commented Oct 18, 2016

@dougwilson Thanks.

@thihara
Copy link
Author

thihara commented Oct 20, 2016

@dougwilson Any luck with reconciling this with the other PR?

@dougwilson
Copy link
Contributor

Not yet, but it has not been any time at all, in a place where many people (like myself) do this on our own spare time. For example, if someone asked a question in my repo while I was on a nice one week vacation, expecting me to respond instantly is not very fair. You're always welcome to comment on the linked PR as well.

@thihara
Copy link
Author

thihara commented Oct 20, 2016

@dougwilson Understood. I'm kind of under a looming deadline so I published a new npm module to bypass this until a resolution is met. But like I said, I'm not optimistic about the response time in the node-mime repository. :-\

@dougwilson
Copy link
Contributor

Sure, but there hasn't even been enough time to know. I have spoken with the author various time, and we even worked together to form the mime-db module this depends on. I don't think it's fair to assume that there will be no response.

If you are "under a deadline", there are a few things you can do now:

  1. Just add your repo to your package.json directly.
  2. Since we already export (and document) the .extensions map, the only thing you even need to do is clean up the MIME type string, if you even need to. A very simple way to do this PR user-land:
var mime = require('mime-types')

module.exports = Object.create(mime)
module.exports.allExtensions = function (str) {
  var type = /^\s*([^;\s]*)(?:;|\s|$)/.exec(str)
  return type && type[1] && mime.extensions[type[1].toLowerCase()] || false
}

Drop that into a file and require() it instead of mime-types directly.

@thihara
Copy link
Author

thihara commented Oct 20, 2016

@dougwilson My observation stemmed from the duration the pull request you referred to me has been open. But if you have personal experience, then that's great, hopefully you will get a response soon. Thanks, I've already published the new module and used it. I'll get rid of it when this is resolved. 👍

@dougwilson
Copy link
Contributor

dougwilson commented Oct 20, 2016

I've already published the new module and used it. I'll get rid of it when this is resolved.

Ah, didn't even realize :) It's not possible to remove modules from npm any longer, since the left-pad incident, afaik.

@thihara
Copy link
Author

thihara commented Oct 20, 2016

@dougwilson Really? Oh I didn't realize, I thought npm unpublish --force would do the trick.

@dougwilson
Copy link
Contributor

Ah, guess I didn't remember right. According to http://blog.npmjs.org/post/141905368000/changes-to-npms-unpublish-policy you can unpublish within 24 hours via command line, otherwise contact support as long as nothing is depending on the module or whatever :)

@dougwilson
Copy link
Contributor

Anyway, that's not to say publishing is bad, haha, I guess we're getting off topic :)

@dougwilson dougwilson added this to the 2.0 milestone Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants