Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

#10071 bash detection for extensionless file #10141

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

Conversation

gklo
Copy link

@gklo gklo commented Dec 10, 2014

Hi, this is probably not the best solution. Please review my code and let me know :)

@dangoor
Copy link
Contributor

dangoor commented Dec 10, 2014

drive-by comment: the language modes themselves should define how to detect them via the text of the document.

@dangoor
Copy link
Contributor

dangoor commented Dec 10, 2014

(In other words, language manager should not be hardcoded to detect bash scripts... a bash mode, likely defined in an extension, would detect that.)

@gklo
Copy link
Author

gklo commented Dec 10, 2014

Should I add an attribute like autoDetections: ['#!/bin/sh', '#!/bin/bash', '#!/usr/bin/env bash'] into languages.json?

@gklo
Copy link
Author

gklo commented Dec 10, 2014

It might be tricky since every language needs different code for detection

@dangoor
Copy link
Contributor

dangoor commented Dec 10, 2014

You could think about possibly using a regex. And that's right that every language needs to do something different to detect, which is why it belongs in the language mode definition.

Also, the search could probably be restricted to the first line of the file.

BTW, you'll need to sign the CLA before we could accept any pull requests. I'll also note that we have a large backlog of pull requests at the moment, so I can't guarantee how quickly we'll get to this one... sorry about that.

@gklo
Copy link
Author

gklo commented Dec 10, 2014

It's okay, thanks a lot! I signed the CLA. I will change my code and push it to see.

@gklo
Copy link
Author

gklo commented Jan 8, 2015

Since extensionless files are likely to be script, so I re-implement it as general shebang detection instead of bash specifed. It might have better performance as well because no need to go through each language object to test regexp. Thanks to @apla, I have a clear idea of using regexp now.

*/
Document.prototype._updateLanguage = function () {
Document.prototype._updateLanguage = function (rawText) {
Copy link
Member

Choose a reason for hiding this comment

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

This is called from other places that won't pass rawText (e.g. _notifyFilePathChanged(), DocumentManager._handleLanguageAdded(), etc.). Will the behavior be correct then? It safer & simpler to remove the arg and just invoke this.getText() instead.

Copy link
Member

Choose a reason for hiding this comment

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

But also... since this is called in multiple cases, various actions will trigger a "re-check" of the document's text. If the user edits the document to add/remove the shebang, at what point would they expect the document to switch to/from Bash mode? We should make sure that feels predictable.

Copy link
Author

Choose a reason for hiding this comment

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

I believe this.getText() won't exist cause the functions are called by Document constructor. Therefore, using rawText is probably the best way to pass the contents to LanguageManager. So far, the shebang detection happens only when user opens a file, I am not sure about the "re-check" triggers during editing. Could you or anybody help me for that?

Copy link
Author

Choose a reason for hiding this comment

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

... and yes rawText can be null, so _updateLanguage() can be called without rawText.

@nextgenthemes
Copy link

I really would appreciate this or a extension, I am tired of manually switching to bash. Well I will probably rename all my scripts to .sh now even if I not really like it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants