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

[meta] Add findModeByFileName, let all meta functions accept uppercase #3027

Closed
wants to merge 1 commit into from

Conversation

silverwind
Copy link
Contributor

As discussed in #2940, this adds findModeByFileName which takes a filename and checks each mode in modeInfo for a new property file, which contains a regexp to test against the given filename. If no filename matches, it tries to extract a extension and fall back to findModeByExtension.

I also saw the opportunity to let all four meta functions accept data in upper and mixed case too.

Finally, the modeInfo has been extended with filenames, and extensions.This is the result of me googling up modes with no extension associated. There are still modes with no mime or extension, but adding some to these would've been plain guessing.

@mattpass
Copy link
Contributor

This looks interesting but a couple of the filetypes shouldn't be there?

GitHub Flavored Markdown - isn't just README.md but could be anything with a .md extension such as CONTRIBUTING.md or HISTORY.md
Nginx - can include other .conf files too thru the use of includes say include vhosts/more-sites.conf or wildcards (`*.conf``)

The other filetype values (Asterisk etc) I'm not sure on but the above I am.

@silverwind
Copy link
Contributor Author

GitHub Flavored Markdown - isn't just README.md but could be anything with a .md extension such as CONTRIBUTING.md or HISTORY.md

README.md is the only one rendered by default on Github as far as I know. I could see adding others here, but I wonder if these are just loosely based on convention or if there's a defininte list.

Nginx - can include other .conf files too thru the use of includes say include vhosts/more-sites.conf or wildcards (*.conf`)

I first thought of allowing paths and using regexp pattern to match stuff like the common names for vhosts files, but decided against it for simplicty. I could easily extend it if requested.

@mattpass
Copy link
Contributor

GitHub will render any file ending with the .md extension. See examples here: https://github.com/icetbr/markdown-examples

You also couldn't list Nginx conf files as they too could be any name with the extension .conf

Just don't think you can name specific filenames for either of these languages.

@silverwind
Copy link
Contributor Author

GitHub will render any file ending with the .md extension.

Yeah, but what I meant is, only a README.md is displayed below a directory's root, making it kind of special. I just went ahead and added your suggested filenames to gfm.

You also couldn't list Nginx conf files as they too could be any name with the extension .conf

If there's path support, I could possibly use /nginx[\/\\]\.conf$/ and get a good success rate because these conf files most likely reside in /etc/nginx. But I think that would require a findModeByPath which would then fall through to findModeByFileName, just like I do for extensions. I've changed the nginx filename to act like *nginx*.conf now.

Paths would probably a very rare use case, and probably not worth the extra weigth it adds to CM. While I could myself use it, most commonly you only have a filename available in a web app.

@silverwind silverwind force-pushed the meta-improvements branch 3 times, most recently from 598472a to 5e23be1 Compare January 11, 2015 21:28
@hitsthings
Copy link
Contributor

Just wanted to voice that I also think you should just add the .md extension instead of specific filenames. Nothing about the filename describes the flavor of Markdown, and .md files will always be Markdown.

I like this patch overall, thanks for it! Means we can simplify some of our own code that worked similarly.

@silverwind
Copy link
Contributor Author

Just wanted to voice that I also think you should just add the .md extension instead of specific filenames.

I would if i could, but the extension is already mapped to the vanilla markdown mode. If we can agree that Github Markdown is more popular, we could remap the extension.

@hitsthings
Copy link
Contributor

Ah in that case, I'm the opposite. :) Idremove the added filenames entirely. There is really nothing about those filenames that says their flavor.

@silverwind silverwind force-pushed the meta-improvements branch 2 times, most recently from fb49ac0 to 1aa5d82 Compare January 11, 2015 23:57
@silverwind
Copy link
Contributor Author

Reduced the function a bit. Filenames are defined by a single regexp now.

@marijnh
Copy link
Member

marijnh commented Jan 12, 2015

I think this looks good as-is, except that I don't think case-insensitivity should be hard-coded into the function. Rather, add an /i to the regexps that need to be case-insensitive.

@silverwind
Copy link
Contributor Author

Will do later today.

For the case of Markdown (if you agree), I think it's the best possible guess we can make. GitHub-based filenames are pretty common, and (if I remember correctly) the difference between regular and GitHub markdown are subtle enough, so it will render mostly correct in the wrong mode too.

@silverwind
Copy link
Contributor Author

Updated and squashed the commits. Case sensitivity now depends entirely on the regexp.

Also changed Dockerfile to be case-sensitive.

@marijnh
Copy link
Member

marijnh commented Jan 13, 2015

Thank you! Merged as 78e06ac. I've removed the line that downcases extensions by default. I understand the rationale, but I've seen people use uppercase letters in extensions in a significant way, so I don't think this is a decision that should be forced on all users of the library.

@marijnh marijnh closed this Jan 13, 2015
@silverwind
Copy link
Contributor Author

Awesome, thanks!

dshoreman pushed a commit to dshoreman/servidor that referenced this pull request Oct 12, 2020
Replaces `CodeMirror.findModeByExtension` with `findModeByFileName` to
detect nginx config files properly. If `CodeMirror.findModeByFileName`
doesn't match anything it automatically tries to find a match via
`findModeByExtension`. If that fails to return info as well, we try to
determine a mode by calling `CodeMirror.findModeByMIME`.

This is the discussion + pr in the CodeMirror repo:
* codemirror/codemirror5#2940
* codemirror/codemirror5#3027

Fixes #187
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.

4 participants