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

Add Svelte as a syntax #1285

Merged
merged 4 commits into from
Oct 29, 2020
Merged

Add Svelte as a syntax #1285

merged 4 commits into from
Oct 29, 2020

Conversation

kjmph
Copy link
Contributor

@kjmph kjmph commented Oct 7, 2020

Another syntax definition that is helpful for bat to incorporate.

@eth-p
Copy link
Collaborator

eth-p commented Oct 7, 2020

Before we add built-in syntaxes to bat, they need to meet a minimum requirement of installs on PackageControl.

Here are the metrics I found:

While the only metric we go by currently is PackageControl (dicussion: #1262), the number of JetBrains installs easily triples our minimum criteria... @sharkdp, @keith-hall: thoughts?

@kjmph
Copy link
Contributor Author

kjmph commented Oct 7, 2020

Thanks, let me know if I can help. Btw, when you say "add built-in syntaxes," should I interpret that there is a way to have syntaxes added via plugins, and not as a submodule?

@keith-hall
Copy link
Collaborator

Interesting that the syntax is most popular with JetBeans :) nice detective work, @eth-p. I say we can allow it.

Btw, when you say "add built-in syntaxes," should I interpret that there is a way to have syntaxes added via plugins, and not as a submodule?

@kjmph It is possible to customize bat to use syntax definitions not included by default in the installation. Here we are just discussing whether we want Svelte included in the installation by default. Having a plugin system to provide more syntaxes is something we are actively discussing, but doesn't exist yet.

@kjmph
Copy link
Contributor Author

kjmph commented Oct 7, 2020

Ahh, I see now. I'll change my workflow and start using the cache build more effectively. Thanks for that pointer.

@sharkdp
Copy link
Owner

sharkdp commented Oct 9, 2020

the number of JetBrains installs easily triples our minimum criteria...

Ok, but I don't think these numbers are comparable. Maybe JetBrains auto-downloads the syntax package every month for update checks (just as an example). Or maybe there are much more or much less JetBrains users compared to Sublime users.

On the other hand, I think there are definitely some syntaxes that would not make sense for Sublime Text users to install.. but would still be very valuable for bat. A stupid example would be the /proc/cpuinfo syntax. Another example might be a syntax for a language that comes with its own de-facto IDE. Something like Mathematica comes to mind.

Anyhow.. I understand we want to merge this. @kjmph Thank you very much for your contribution!

Could you please add an entry to the "unreleased" section in CHANGELOG.md? The format for entries is:

- Description what has been changed, see #123 (@user)

where #123 links to the bug ticket and/or the PR and user is your username.

@kjmph
Copy link
Contributor Author

kjmph commented Oct 23, 2020

My apologies on the delay. I believe the latest commit takes care of the Changelog request.

@sharkdp
Copy link
Owner

sharkdp commented Oct 24, 2020

@kjmph Thank you for the update. I'm happy to merge this if we can also add a syntax regression test along the lines of #1213.

We should document this as a requirement for future syntax additions.

@kjmph
Copy link
Contributor Author

kjmph commented Oct 27, 2020

I believe this latest commit takes care of the regression syntax test.

Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

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

Thank you!

@sharkdp sharkdp merged commit 072fb38 into sharkdp:master Oct 29, 2020
@sharkdp
Copy link
Owner

sharkdp commented Nov 23, 2020

Added in bat v0.17.

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.

5 participants