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 SaltStack SLS syntax #658

Merged
merged 1 commit into from
Oct 6, 2019
Merged

Add SaltStack SLS syntax #658

merged 1 commit into from
Oct 6, 2019

Conversation

Martin819
Copy link
Contributor

@Martin819 Martin819 commented Sep 9, 2019

Adding new syntax highlighting for SaltStack states - SLS files.

Example:
Screenshot from 2019-09-09 15-27-47

@sharkdp
Copy link
Owner

sharkdp commented Sep 14, 2019

Thank you for your contribution.

I've never heard of SaltStack before. It sounds like this syntax would be something very specific. Have you considered adding this syntax to your local installation?

@Martin819
Copy link
Contributor Author

Martin819 commented Sep 17, 2019

Hi @sharkdp, I already have it my local installation and it works just fine and that's why I felt it would be great if bat would include it by default.
SaltStack is an open-source configuration management software, very similar to Ansible. It uses SLS files which are in fact conbination of YAML and Jinja2. The problem is that either YAML nor Jinja2 syntax highlighting works for these files. That's why SaltStack itself came with this syntax highlight which wraps around Jinja2 highlight and enables it to be used on YAML+Jinja2 files.

You can see in the following examples how it fixes highlighting:

YAML syntax forced:
Screenshot from 2019-09-17 13-21-50

Jinja2 syntax forced:
Screenshot from 2019-09-17 13-21-27

SLS syntax applied:
Screenshot from 2019-09-09 15-27-47

@sharkdp
Copy link
Owner

sharkdp commented Sep 21, 2019

@eth-p Your opinion?

Maybe we should add some policy to make objective decisions on similar PRs that add new syntaxes.

SublimeText package is here:
https://packagecontrol.io/packages/SaltStack-related%20syntax%20highlighting%20and%20snippets

@eth-p
Copy link
Collaborator

eth-p commented Sep 27, 2019

@sharkdp We should definitely make some kind of objective policy for adding new syntaxes. The hard part is going to be figuring out how to gather metrics on the proposed language, though.

While SublimeText is a decent way of figuring out how many people use both Sublime and the language, I feel like it could be missing out on a potentially significant portion of users who don't actually use Sublime.

Some other metrics we could use:

  • StackOverflow (survey, tag count, etc.)
  • Atom plugins (download count)
  • JetBrains language plugins (download count)

Additionally, what do you think would be a good "minimum" number of downloads for a language to be considered popular enough that it should be added?

@sharkdp
Copy link
Owner

sharkdp commented Sep 30, 2019

Some other metrics we could use: [...]

That's certainly a good idea!

Additionally, what do you think would be a good "minimum" number of downloads for a language to be considered popular enough that it should be added?

So far, my threshold was somewhere between 10k and 20k total downloads. But I guess I wasn't strictly following that.

@eminence
Copy link

eminence commented Oct 3, 2019

Another possibility might be to use the list of languages that github itself will syntax highlight: https://github.com/github/linguist/blob/master/vendor/README.md

@Martin819
Copy link
Contributor Author

Martin819 commented Oct 4, 2019

Hi, regarding the questions on the need for this language, there are some results:

  • over 2000 questions with tags "salt-stack" or "salt" --> LINK

  • Atom plugin available --> LINK

  • Not available for Jetbrains, but that's not a surprise since Salt is a cloud management service, you edit the files in 99% cases in CLI and not in the UI based IDEs.

  • GitHub syntax is available --> LINK(from README file from previous post)

  • SaltStack GitHub repo - more than 10,000 users starred, 4,600 forks, over 20,000 issues created, over 33,000 PRs created - this is a very used piece of software. --> LINK

@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2019

@Martin819 Thank you for the detailed update! Let's merge this for now.

If we have a problem with too many syntax being bundled with bat in the future, I guess it's not too bad to remove some of them again - even though it is a regression of some sort.

@sharkdp sharkdp merged commit f0f77b1 into sharkdp:master Oct 6, 2019
@sharkdp
Copy link
Owner

sharkdp commented Oct 6, 2019

(nightly builds are currently broken, but this has nothing to do with this PR)

@sharkdp
Copy link
Owner

sharkdp commented Mar 22, 2020

This has been released in bat 0.13.

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