[CLOSED] Fixing issue #2923 - Getting mode from file extension won't always work

[core-ai-bot]

Issue by MarkMurphy
Sunday Mar 03, 2013 at 15:53 GMT
Originally opened as adobe/brackets#3029

---

Fixed issue #2923 Getting mode from file extension won't always work where some files don't have extensions like Makefiles for example.

---

MarkMurphy included the following code: https://github.com/adobe/brackets/pull/3029/commits

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 04, 2013 at 17:13 GMT

---

I like this approach... certainly seems to be needed.

[core-ai-bot]

Comment by MarkMurphy
Monday Mar 04, 2013 at 17:33 GMT

---

@DennisKehrig I agree actually. I'll rename the getLanguageFromFilePath to getLanguageFromPath and fix the other issues later today.

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 04, 2013 at 17:47 GMT

---

Happy to hear that! Can it be "for" instead of "from", though? The latter suggests to me that the language is magically contained in the path, whereas this really is a rather loose association.

[core-ai-bot]

Comment by MarkMurphy
Monday Mar 04, 2013 at 19:24 GMT

---

@DennisKehrig hmm, not sure I'm convinced... because the language kind of is "magically" contained in the path. That is after all how we're determining what language to use. Unless I misunderstood what you meant by "this" I also don't think that it should be a loose association. If anything it should be somewhat strict.

[core-ai-bot]

Comment by MarkMurphy
Tuesday Mar 05, 2013 at 01:17 GMT

---

Anything else I need to do before this can be merged?

[core-ai-bot]

Comment by DennisKehrig
Tuesday Mar 05, 2013 at 08:51 GMT

---

@MarkMurphy Not a blocker on my end, but let me have another shot at getLanguageForPath instead of getLanguageFromPath :)

We will at some point allow the user to set the language for a file themselves. Probably not just for a file extension, but directly for one file. At that point, the path becomes nothing but an arbitrary lookup string, whose contents have nothing to do with the language object returned.

On a different angle, I think we're not just getting something that is close to the file name - language ID from a file path, say "css" from "foo.css", but we're getting a whole language object that (eventually) describes a lot more, like the comment syntax. I can't get that "from" the file name, but I can get that "for" the file name.

Do you receive value for money or from money? Usually you buy goods with money, and that is the value. Getting value from money would be more like heating your home by burning it or using a coin as a screw driver or something - the inherent value.

Now I admit that this is a rather philosophical debate for such a small issue, but I'm having fun here ^^

[core-ai-bot]

Comment by MarkMurphy
Tuesday Mar 05, 2013 at 14:38 GMT

---

@DennisKehrig I didn't initially perceive it that way which is why I named it the way I did, it was just more intuitive in my mind that way but I see where you're coming from now however it still may be more personal preference than anything. I'm open to changing it but I'd like to have at least one other opinion on it before I act.@peterflynn what are your thoughts on this?

[core-ai-bot]

Comment by DennisKehrig
Tuesday Mar 05, 2013 at 14:43 GMT

---

@MarkMurphy Fair enough, thank you for considering it! It certainly will work fine either way.

[core-ai-bot]

Comment by MarkMurphy
Wednesday Mar 06, 2013 at 20:19 GMT

---

@DennisKehrig@peterflynn is a pretty busy dude so I'm just going to make the change. I thought about it some more and I think it makes sense to change it especially because it will make it a little more consistent with the previous version of this function. I'll push that up later today.

[core-ai-bot]

Comment by DennisKehrig
Wednesday Mar 06, 2013 at 20:30 GMT

---

@MarkMurphy Awesome, happy to hear that! :)
BTW: today I also stumbled over that bug where only passing an extension to getLanguageForFileExtension doesn't work. As you saw when you created your fix, so far we only called the function with full path names, apparently.

[core-ai-bot]

Comment by peterflynn
Thursday Mar 07, 2013 at 02:56 GMT

---

@MarkMurphy fwiw I don't have a strong opinion either way on the name. Whatever you guys decide is best.

[core-ai-bot]

Comment by MarkMurphy
Thursday Mar 07, 2013 at 03:04 GMT

---

@peterflynn Cool, well I think this one is ready to go.

[core-ai-bot]

Comment by MarkMurphy
Thursday Mar 07, 2013 at 03:13 GMT

---

@DennisKehrig Yeah I found that when I was updating calls to getLanguageFromFileExtension, I didn't really think about it at the time but yeah... fixed now.

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 11, 2013 at 11:14 GMT

---

@MarkMurphy There are still some references to getLanguageForFileExtension in test\spec\Editor-test.js and test\spec\LanguageManager-test.js, can you fix those as well? I think then this is ready to go!

@peterflynn Since I'm assigned, I should be the one to merge it, correct?

[core-ai-bot]

Comment by MarkMurphy
Monday Mar 11, 2013 at 13:24 GMT

---

@DennisKehrig good catch, not sure how I missed those. All fixed.

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 11, 2013 at 16:42 GMT

---

Happens to me all the time, I usually only search in the src folder, only recently I got more aware of the test files. :)

Thanks a lot! Waiting for@peterflynn to confirm my merging rights (then I'll check out your branch and test)

[core-ai-bot]

Comment by peterflynn
Monday Mar 11, 2013 at 19:02 GMT

---

@DennisKehrig yep, it's all in your hands! I feel like my comments were addressed well enough.

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 11, 2013 at 19:10 GMT

---

@peterflynn Great, thank you!

[core-ai-bot]

Comment by DennisKehrig
Monday Mar 11, 2013 at 19:12 GMT

---

@MarkMurphy I'll get to this on Wednesday! Some of my pull requests will likely need rebasing after merging this, so I'll need more time than I have now.

[core-ai-bot]

Comment by DennisKehrig
Wednesday Mar 13, 2013 at 12:08 GMT

---

@MarkMurphy Merged it! I was a bit too eager, though, forgot to go through the review checklist BEFORE clicking the button. I fixed a couple of things - missing documentation, missing unit tests, JSLint errors and the fact that file names were not converted to lowercase when defining them or looking them up.

Thanks for getting this started!

[core-ai-bot]

Comment by MarkMurphy
Wednesday Mar 13, 2013 at 13:24 GMT

---

@DennisKehrig Thanks! I'll be sure to remember those things for next time. I initially did convert the filenames to lowercase but then decided to leave them as they were because I thought there may be some scenarios where case sensitivity matters.

[core-ai-bot]

Comment by DennisKehrig
Wednesday Mar 13, 2013 at 13:35 GMT

---

That would be awesome! Though in general I would not expect everybody to add unit tests.

I actually thought you might have considered lower-casing it. #3122 will enhance things a bit, though, so that even file names with extensions can be inserted into the fileNames array - to me the name doesn't clearly communicate that it has to be a file name without an extension. By then we're definitely in a place where it's more likely that someone wants this case-insensitive rather than complaining about "MAKEfile" not being displayed as a simple text file. But just like you said, we might be overlooking a scenario.