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

Consolidate JS, TS, LiveScript, CoffeeScript on common foundation #2518

Merged
merged 2 commits into from Apr 29, 2020

Conversation

joshgoebel
Copy link
Member

@joshgoebel joshgoebel commented Apr 28, 2020

Just the beginning, but it's a start.

@joshgoebel joshgoebel changed the title WIP: Consolidate JS and TS on common foundation Consolidate JS and TS on common foundation Apr 28, 2020
@joshgoebel joshgoebel changed the title Consolidate JS and TS on common foundation Consolidate JS, TS, LiveScript on common foundation Apr 28, 2020
@joshgoebel joshgoebel added this to the 10.1 milestone Apr 28, 2020
@joshgoebel joshgoebel changed the title Consolidate JS, TS, LiveScript on common foundation Consolidate JS, TS, LiveScript, CoffeeScript on common foundation Apr 28, 2020
Copy link
Collaborator

@egor-rogov egor-rogov left a comment

Choose a reason for hiding this comment

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

I like the idea.
I didn't check all the keywords though, hope you did it right.

src/lib/modes.js Outdated Show resolved Hide resolved
className: 'meta',
begin: /^#![^\n]+sh\s*$/,
const SHEBANG = hljs.SHEBANG({
binary: /(fish|bash|zsh|sh)/,
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are more... What's wrong with previous .+sh approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

Then the relevance would have to change.. not every binary ending in sh is a shell... If we went a high relevance then we should have an explicit list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure what are the most frequently used shells (except for bash and historically sh, of course). Probably your list is good enough, I just don't know.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you'd like to suggest a few more to add, we have the room. :-) I'd rather add them slowly than have false positives. And if your concern is just highlighting the shebang itself (vs treating it as a comment) we could add a basic SHEBANG rule also with 0 relevancy - to catch shell like files with an unknown binary...

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't look at that as a "match any *sh" I read that as match the path then sh... but you're right that before it would have been more generic. I guess I just dislike that - at least when it comes to appointing 10 relevancy.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you'd like to suggest a few more to add, we have the room. :-)

I've heard of, say, ksh, but I have no idea if someone use it these days. I have no must-have suggestions.

And if your concern is just highlighting the shebang itself (vs treating it as a comment) we could add a basic SHEBANG rule also with 0 relevancy - to catch shell like files with an unknown binary...

I think it's a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

I expanded the list:

"fish",
    "bash",
    "zsh",
    "sh",
    "csh",
    "ksh",
    "tcsh",
    "dash",
    "scsh",

Copy link
Collaborator

@egor-rogov egor-rogov left a comment

Choose a reason for hiding this comment

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

Looks good for me now.

Builds a common keyword foundation for any grammar that is
building on top of JavaScript:

- LiveScript
- CoffeeScript
- TypeScript

Also uses this common foundation for JS itself.
@joshgoebel joshgoebel merged commit a23f19e into highlightjs:master Apr 29, 2020
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.

None yet

2 participants