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

[MarkMurphy]

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

[DennisKehrig]

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

[MarkMurphy]

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

[DennisKehrig]

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.

[MarkMurphy]

@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.

[peterflynn]

Not sure we need to do anything about this now, but it occurs to me that we don't have a good story for how an extension would offer improved support for these non-generic forms of XML. If the generic "xml" language has already claimed that file extension, no one can add a new language for it.

So for example, if I wanted to make an extension that offers offers tag & attribute hinting for SVG I'd ideally be able to un-register ".svg" from the "xml" language and declare a new "svg" language to take over it. (For code hinting specifically, you could do hacks to avoid defining a new language, but ultimately that approach will probably hit a wall).

This is relevant here since the more specific XML formats we add to our generic XML language, the more likely this is to become an issue. But as with my SVG example above, it's possible to hit this issue even on current master.

[DennisKehrig]

You're absolutely right. I wonder whether we'd want extensions to be able to simply add a file extension, and have them remove the file extension from some other language, or whether extensions have to prove some kind of awareness of the issue, like calling a special replaceFileExtension method somewhere, or having to provide a language from which they want to take a file extension, to indicate that this is a conscious specialization of a file extension.

[MarkMurphy]

Agreed. Started a new issue around this. Lets move the discussion there: Issue #3044

[MarkMurphy]

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

[DennisKehrig]

@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 ^^

[MarkMurphy]

@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?

[DennisKehrig]

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

[MarkMurphy]

@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.

[DennisKehrig]

@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.

[peterflynn]

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

[MarkMurphy]

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

[MarkMurphy]

@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.

[DennisKehrig]

@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?

[MarkMurphy]

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

[DennisKehrig]

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)

[peterflynn]

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

[DennisKehrig]

@peterflynn Great, thank you!

[DennisKehrig]

@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.

[DennisKehrig]

@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!

[MarkMurphy]

@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.

[DennisKehrig]

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.