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.

Sign up for GitHub

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

Jump to bottom

Add support for tmux #1882

Closed
wants to merge 8 commits into from
Closed

Add support for tmux #1882

wants to merge 8 commits into from

Conversation

cbolgiano
Copy link
Contributor

@cbolgiano cbolgiano commented Oct 3, 2021 •
edited
Loading

Adds support for Tmux.

@cbolgiano
Copy link
Contributor Author

Will run fmt shortly.

SuperSandro2000
SuperSandro2000 reviewed Oct 3, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@cbolgiano
Copy link
Contributor Author

cbolgiano commented Oct 6, 2021 •
edited
Loading

I am almost ready to push an update. I love this project. It is so cool.

@cbolgiano
Copy link
Contributor Author

Latest work has been pushed.

@cbolgiano
Copy link
Contributor Author

cbolgiano commented Oct 6, 2021 •
edited
Loading

Looking at the raw log of the regression tests this is what I see:

2021-10-06T04:25:43.0189536Z === Error: Could not highlight source file '/home/runner/work/bat/bat/tests/syntax-tests/source/DotENV/.env
2021-10-06T04:25:43.0190260Z === bat stdout:
2021-10-06T04:25:43.0190512Z
2021-10-06T04:25:43.0190809Z === bat stderr:
2021-10-06T04:25:46.9500990Z thread 'main' panicked at 'called Result::unwrap() on an Err value: Ok(SyntaxReferenceInSet { syntax: SyntaxReference { name: "resolv", file_extensions: ["resolv.conf"], scope: <source.resolv>

^ I am new to rust but it seems like I need to wrap the Error in Err here: https://github.com/sharkdp/bat/pull/1882/files#diff-0a104d0fbbdd3652151d2d31956b1fbcbc2b2c14163d02395d765a82aff299abR194. Going to try that to fix the failing regression tests. If I am incorrect and there is something else I introduced please let me know.

@Enselic
Copy link
Collaborator

Enselic commented Oct 6, 2021

Thanks for the update! I don’t have time to deep dive into the code right now, but I glanced it over, and have these spontaneous comments:

  • would you be ok with splitting this PR up into two? One for adding tmux and one for adding MapToUnknownUnlessExactFileNameMatch? That will make code review and everything else easier.
  • Rather than adding more calls to self.get_extension_syntax, try to split it up and rearrange. The goal is to only do each thing once. I think that can be achieved by different ordering.

@cbolgiano
Copy link
Contributor Author

Absolutely. I don't mind. I will work on that soon. Thank you!

@cbolgiano
Copy link
Contributor Author

cbolgiano commented Oct 6, 2021 •
edited
Loading

Moved the implementation of MapToUnknownExtension to a different MR: #1889

@cbolgiano cbolgiano requested a review from Enselic October 7, 2021 21:15
@cbolgiano
Update main.rs
a1a069c 
Add  #![deny(unsafe_code)] back
@cbolgiano cbolgiano changed the title 1703: Add support for tmux Add support for tmux Oct 8, 2021
@cbolgiano
Copy link
Contributor Author

Please let me know if there is anything else I can do in order to get this accepted and merged. Thank you!

@Enselic
Copy link
Collaborator

Enselic commented Oct 10, 2021

Let’s wait with this one until the generic solution is in place

@cbolgiano cbolgiano mentioned this pull request Oct 19, 2021
Merged
MichaelCurrin
MichaelCurrin reviewed Oct 31, 2021
View reviewed changes
assets/syntaxes/02_Extra/Tmux/README.md

Close Sublime Text then download or clone this repository to a directory named `Tmux` in the Sublime Text Packages directory for your platform:

* Linux: `git clone https://github.com/gerardroche/sublime-tmux.git ~/.config/sublime-text-3/Packages/Tmux`
Copy link
Contributor

@MichaelCurrin MichaelCurrin Oct 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
* Linux: `git clone https://github.com/gerardroche/sublime-tmux.git ~/.config/sublime-text-3/Packages/Tmux`
* Linux:
```bash
git clone https://github.com/gerardroche/sublime-tmux.git ~/.config/sublime-text-3/Packages/Tmux
```

MichaelCurrin
MichaelCurrin reviewed Oct 31, 2021
View reviewed changes
assets/syntaxes/02_Extra/Tmux/README.md
Close Sublime Text then download or clone this repository to a directory named `Tmux` in the Sublime Text Packages directory for your platform:

* Linux: `git clone https://github.com/gerardroche/sublime-tmux.git ~/.config/sublime-text-3/Packages/Tmux`
* OSX: `git clone https://github.com/gerardroche/sublime-tmux.git ~/Library/Application\ Support/Sublime\ Text\ 3/Packages/Tmux`
Copy link
Contributor

Choose a reason for hiding this comment

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

"OSX" is the older systems. Apple uses "macOS" for their newer systems.

Enselic
Enselic requested changes Oct 31, 2021
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

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

Looks like the sub module is not actually added like a sub module like the other syntaxes?

Also, how many downloads does this syntax have on package control?

@Enselic
Copy link
Collaborator

Enselic commented Nov 20, 2021

Looks like tmux is quite far from 10k?

This PR was more of a stepping stone to #1889 I suppose.

Big thanks again for that already legendary PR btw 🙂

I think we can go ahead and close this PR though.

@Enselic Enselic closed this Nov 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelCurrin MichaelCurrin MichaelCurrin left review comments

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@Enselic Enselic Enselic requested changes

Assignees
No one assigned
Labels
needs-work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cbolgiano @Enselic @SuperSandro2000 @MichaelCurrin