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 Requirementstxt.sublime-syntax and its test. #2361

Merged
merged 1 commit into from
Nov 2, 2022
Merged

Add Requirementstxt.sublime-syntax and its test. #2361

merged 1 commit into from
Nov 2, 2022

Conversation

Freed-Wu
Copy link
Contributor

@Freed-Wu Freed-Wu commented Oct 7, 2022

❯ syntest assets/syntaxes/02_Extra/syntax_test_requirements.txt assets/syntaxes/02_Extra/Requirementstxt.sublime-syntax
loading syntax definitions from assets/syntaxes/02_Extra/Requirementstxt.sublime-syntax
Testing file assets/syntaxes/02_Extra/syntax_test_requirements.txt
The test file references syntax definition file: Requirementstxt.sublime-syntax
Ok(Success(850))
exiting with code 0

https://github.com/raimon49/requirements.txt.vim/tree/master/examples/requirements.txt

Screenshot from 2022-10-07 16-31-49

@sharkdp
Copy link
Owner

sharkdp commented Oct 29, 2022

Thank you very much. We used to have requirements.txt support, but the license was not compatible. This syntax seems fine. Can you please read through https://github.com/sharkdp/bat/blob/master/doc/assets.md and add bat-style syntax tests?

@Freed-Wu
Copy link
Contributor Author

Fixed.

@sharkdp
Copy link
Owner

sharkdp commented Oct 30, 2022

Please also read the part about adding the upstream syntax as a submodule: https://github.com/sharkdp/bat/blob/master/doc/assets.md#adding-new-builtin-languages-for-syntax-highlighting

And remove assets/syntaxes/02_Extra/syntax_test_requirements.txt.

@Freed-Wu
Copy link
Contributor Author

OK, removed.

@sharkdp
Copy link
Owner

sharkdp commented Oct 30, 2022

But we still don't have the upstream syntax as a submodule, right?

@Freed-Wu
Copy link
Contributor Author

But we still don't have the upstream syntax as a submodule, right?

You mean I should create a new github repo and create a git submodule in this repo, right?

@sharkdp
Copy link
Owner

sharkdp commented Oct 30, 2022

But we still don't have the upstream syntax as a submodule, right?

You mean I should create a new github repo and create a git submodule in this repo, right?

Oh, I didn't realize you created this syntax. I saw the link to https://github.com/raimon49/requirements.txt.vim/tree/master/examples/requirements.txt and didn't realize it was a vim syntax, not a Sublime Text syntax. In this case, I'm okay with integrating it into bat directly, but maybe in its own subfolder (where you can also add the sublime syntax test, sorry 😄). But having it in a separate repository would also be great. Whatever you prefer.

@Freed-Wu
Copy link
Contributor Author

you can also add the sublime syntax test, sorry

No serious 😄

maybe in its own subfolder

Why not create a subfolder for every *.sublime-syntax?
Now in assets/syntaxes/02_Extra, folder and file exist at the same time. It looks not very tidy.

@sharkdp
Copy link
Owner

sharkdp commented Oct 30, 2022

Now in assets/syntaxes/02_Extra, folder and file exist at the same time. It looks not very tidy.

We do have folders for all submodules. But for some of them we need a manually converted .sublime-syntax file, because they only support .tmLanguage. If the syntax has a .sublime-syntax in the module itself, there is no additional file.

@Freed-Wu
Copy link
Contributor Author

there is no additional file.

I see. If a sublime-syntax has additional file, It will create a subfolder. So should we create a folder for them?

$ ls assets/syntaxes/02_Extra/syntax_test_*
 assets/syntaxes/02_Extra/syntax_test_csv.csv
 assets/syntaxes/02_Extra/syntax_test_man.man
 assets/syntaxes/02_Extra/syntax_test_tsv.tsv

@sharkdp
Copy link
Owner

sharkdp commented Nov 2, 2022

Sorry, I wasn't aware that we had some syntax tests in assets/syntaxes/02_Extra. In that case, let's merge this as is. We can always clean up later.

Thank you very much.

@sharkdp sharkdp merged commit 4724d50 into sharkdp:master Nov 2, 2022
@Freed-Wu Freed-Wu deleted the main branch November 3, 2022 05:21
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.

2 participants