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

[Request] Imporove bash's colours. #3342

Closed
katzeprior opened this issue Sep 29, 2021 · 7 comments · Fixed by #3344
Closed

[Request] Imporove bash's colours. #3342

katzeprior opened this issue Sep 29, 2021 · 7 comments · Fixed by #3344
Labels
enhancement An enhancement or new feature help welcome Could use help from community language
Milestone

Comments

@katzeprior
Copy link
Contributor

Is your request related to a specific problem you're having?
I'm always frustrated when I write a bash code block but it does not support the gnu coreutils so some of the commands are not highlighted like grep and cat for example.
it also doesn't recognize executables that i installed like nmap or any other program.

The solution you'd prefer / feature you'd like to see added...
A clear and concise description of how you imagine we might solve this (if you have any ideas).
add it to the built_in of bash.js
for the other executables that are not in the gnu coreutils or sh buildins i don't know a easy fix, you could make everything coloured that starts at the beginning of the line also include ./<executable name> or if it comes after the pipe echo "cGxlYXNlIGZpeAo=" | base64 -d so echo and base64 are yellow because currently base64 is not.
gnu core utils

Any alternative solutions you considered...
I don't have an alternative solution.

Additional context...

./test "3"
echo "test"
cat test.txt | grep "lol"

only test and echo are coloured, im missing the cat and grep.
also it would be nice to see the ./ also getting a colour.

@katzeprior katzeprior added enhancement An enhancement or new feature parser labels Sep 29, 2021
@joshgoebel
Copy link
Member

does not support the gnu coreutils

How long would this list be?

you could make everything coloured that starts at the beginning of the line

We could. Are there no cases where that would lead to false positives?

or if it comes after the pipe

Neat idea, I think the new multi-matcher stuff could help here since we'd want to highlight | as an operator too probably.

@katzeprior
Copy link
Contributor Author

katzeprior commented Sep 29, 2021

How long would this list be?

the gnu core utils would be 104 items as listed on the wikipedia page, there might be more.
list of unix commands are more that might be missing.

We could. Are there no cases where that would lead to false positives?

There is a # which is a comment, there is also assigning variables example: hello='World!'
I am including some images from my .bashrc that might show some weird edge cases or false positives.
https://imgur.com/a/OLhBWO9

edit:
this image is a side by side view of joplin's markdown (left) and the rendered markdown (right)

@joshgoebel
Copy link
Member

So we'd need to exclude:

  • variable assignment
  • statements (if, case, etc)

I imagine the rule would be (roughly) something like ^[a-z][a-z0-9]+, so we wouldn't need to worry about # at all...

I'd welcome a PR exploring some of these ideas, perhaps starting with just the GNU core utils list...

@katzeprior
Copy link
Contributor Author

my first PR is going better then expected, running into one issue with the tests, i fixed the bash tests to reflect the changes i made but im getting some issues in the dockerfile test and shell test because both have <span class="language-bash"> in the expected output and have a new built_in command in that span.

I counted a total of 3 keywords where only 1 of them should be updated to reflect the changes the other 2 should not be included in the parsing with bash.

for shell:
command-continuation.txt:10
it recognizes in /bin/cat the cat part as a command which it should exclude or capture the whole part maybe.

for dockerfile:
default.expect.txt:10
it recognizes in &amp;&amp; apt-get install -y install as a command which it should exclude because its an argument for apt.
default.expect.txt:11
it recognizes in <span class="language-bash"> mkdir /tmp/sessions</span> that mkdir is a command and thus this could be highlighted but i don't know if this is needed.

@joshgoebel
Copy link
Member

but im getting some issues in the dockerfile test and shell test

If those embed bash then they would need to be updated also to reflect the new changes to bash.

it recognizes in /bin/cat the cat part as a command which it should exclude or capture the whole part maybe.

Yes, we'd need a mode to capture full paths... likely just a variant of whatever rule we use to capture commands at the beginning...

it recognizes in && apt-get install -y install as a command

There is no way to not recognized keywords, so if the commands aren't unambiguous then we may not be able to use them with the keywords system.

that mkdir is a command

Not sure why this is an issue, it is effectively a command in this case, no?

@katzeprior
Copy link
Contributor Author

Would it be enough to change the test so the mkdir is reflecting the new changes and leave the other 2 issues for a future change and make send the pull request of what i have currently?

@joshgoebel
Copy link
Member

Just submit what you having passing or failing and let me have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature help welcome Could use help from community language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants