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 support for dash shebang #1654

Merged
merged 4 commits into from
May 15, 2021
Merged

Add support for dash shebang #1654

merged 4 commits into from
May 15, 2021

Conversation

mohamed-abdelnour
Copy link
Contributor

Hello, everyone, first-timer here!

This is a patch to Bash.sublime-syntax that adds support for shebangs explicitly calling for dash, e.g., #!/usr/bin/env dash.

This is my first time contributing to any project, so I have a couple of questions that may be obvious:

  • In CONTRIBUTING.md it is mentioned that one should update the CHANGELOG. This is done after a pull request is merged, right? Is it a second, separate pull request?
  • Should I add test files (Complete collection of syntax highlighting test files #1213) with dash shebangs, or is it redundant given that this uses the existing Bash syntax file?

I have also prepared a second patch that adds support for XAML files (#1590), but I will wait for feedback on this before submitting another pull request in case I am doing something wrong.

Thank you for your incredible work! I look forward to trying to help out in the future, hopefully with a little bit of Rust!

@keith-hall
Copy link
Collaborator

Thanks for your contribution.

This is my first time contributing to any project, so I have a couple of questions that may be obvious:

* In `CONTRIBUTING.md` it is mentioned that one should update the `CHANGELOG`. This is done _after_ a pull request is merged, right? Is it a second, separate pull request?

* Should I add test files (#1213) with dash shebangs, or is it redundant given that this uses the existing Bash syntax file?

You can update the changelog as part of this pull request :)

I would say that having extra test files is not a bad thing ™️ even if it seems redundant. After all, the dash shell supports less features than bash, and at some point somebody might make a separate syntax definition for it, so it could be useful for regression testing purposes to have a genuine dash script test file.

I think it's also worth making a PR to https://github.com/sublimehq/Packages/blob/master/ShellScript/Bash.sublime-syntax, so that when our syntax highlighting library, syntect, supports the latest features and we can reference the latest upstream version of the syntax, that we wouldn't lose any functionality in bat ;)

Thank you for your incredible work! I look forward to trying to help out in the future, hopefully with a little bit of Rust!

sounds great, nice to have your help! :)

@mohamed-abdelnour
Copy link
Contributor Author

You can update the changelog as part of this pull request :)

I honestly had no idea that adding more commits would automatically reflect on this pull request. Got there in the end! :)

I updated the CHANGELOG and added a test file. It was a little difficult finding a comprehensive script that explicitly calls for dash, so I modified the shebang of a POSIX-compliant shell script and stated that it is a modified version in LICENSE.md.

As a final note, the spelling I used for the test directory and in the CHANGELOG is "dash"—all lowercase. That is how it is spelt in two sources I managed to find:

* Renamed to dash --- the Debian Almquist Shell.

@keith-hall
Copy link
Collaborator

I honestly had no idea that adding more commits would automatically reflect on this pull request. Got there in the end! :)

Apologies, I noticed you said it was your first time contributing to open source, but I didn't pick up on it being your first time with the Pull Request workflow, otherwise I would have given clearer instructions. Glad you figured it out!

It was a little difficult finding a comprehensive script that explicitly calls for dash, so I modified the shebang of a POSIX-compliant shell script and stated that it is a modified version in LICENSE.md.

Great solution :) Thanks for your hard work on this.

@keith-hall
Copy link
Collaborator

It looks like we have some merge conflicts now, which should give you some more experience resolving them :)
Just to note, I missed this earlier, you don't need to include the syntaxes.bin file in the PR. :) this is mentioned at step 7 in https://github.com/sharkdp/bat/blob/master/doc/assets.md

@mohamed-abdelnour
Copy link
Contributor Author

Apologies, I noticed you said it was your first time contributing to open source, but I didn't pick up on it being your first time with the Pull Request workflow, otherwise I would have given clearer instructions. Glad you figured it out!

No need, you saying that I could do it as part of this PR pointed me in the right direction!

Just to note, I missed this earlier, you don't need to include the syntaxes.bin file in the PR. :) this is mentioned at step 7 in https://github.com/sharkdp/bat/blob/master/doc/assets.md

My apologies, I completely missed this. For some reason I thought this process only applied for new syntax files. I have now updated the branch to use syntaxes.bin from master. I made the same mistake in my other PR (#1655), and updated the syntaxes.bin file there, too. This is already merged into master; I am just pointing it out in case the file needs to be reverted. Again, apologies for this mistake, I should have asked prior to committing.

Thank you for your help and patience!

@keith-hall
Copy link
Collaborator

I'm also not sure if we need to revert it or not, hopefully one of the other maintainers will get back to us. But if it does need to be reverted, we'll sort it out, don't worry - thanks for pointing it out. :)

I wonder if it makes sense to have some automated bot warn us about it to help prevent the same thing happening again in future with other PRs :)

@keith-hall keith-hall merged commit bef0bf1 into sharkdp:master May 15, 2021
@mohamed-abdelnour mohamed-abdelnour deleted the support-dash-syntax branch May 15, 2021 08:51
@sharkdp
Copy link
Owner

sharkdp commented May 16, 2021

I'm also not sure if we need to revert it or not, hopefully one of the other maintainers will get back to us.

No need to revert. I added that policy because frequent changes to binary files increase the size of the Git repository (presumably). That's why I usually only update them before releases. But there is no real harm in updating them in between.

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

Successfully merging this pull request may close these issues.

None yet

3 participants