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

Too greedy comment highlighting for bash #1457

Closed
Rychu-Pawel opened this issue Jul 1, 2018 · 8 comments
Closed

Too greedy comment highlighting for bash #1457

Rychu-Pawel opened this issue Jul 1, 2018 · 8 comments

Comments

@Rychu-Pawel
Copy link

Rychu-Pawel commented Jul 1, 2018

I think there is an issue with comment highliting for bash scripts. Please take a look at example on my page: http://pawelrychlicki.pl/Article/Details/58/zfs-health-check-script-for-monit-09

In my opinion lines 57-59, 74-81, 89-93 and 111-131 shouldn't be green. Script is OK as it executes fine. If I put the script into Prism's test drive it looks the same.

@Rychu-Pawel Rychu-Pawel changed the title Too gridy comment highlighty for bash Too greedy comment highlighty for bash Jul 2, 2018
@Rychu-Pawel Rychu-Pawel changed the title Too greedy comment highlighty for bash Too greedy comment highlighting for bash Jul 2, 2018
@Golmote
Copy link
Contributor

Golmote commented Jul 6, 2018

Hi! Thanks for reporting. I think this is a problem with the variable pattern, not the comment one. But there's definitely something wrong starting around line 55.

Would #1443 help there?

@Rychu-Pawel
Copy link
Author

Rychu-Pawel commented Jul 7, 2018

Nope, didn't help. I replaced bash plugin content with the one from prism-bash.js from mentioned PR but highlighting looks exactly the same :(
I created fiddle for it, with bash algorithm from the PR, here: https://jsfiddle.net/c7ak1p82/2/

@alice-mm
Copy link
Contributor

alice-mm commented Jul 8, 2018

I tried to apply a regexp by @Golmote in my PR and it seems to make things better.
image
If you cannot wait for this to be merged or improved or whatever, I guess you can replace

a=$(complicated stuff)

with

function foo {
    complicated stuff
}

a=$(foo)

This should prevent Prism from interpreting the parentheses in your strings as the end of the command substitution, and I think it's a good practice anyway when it comes to scripting as it adds semantics and stuff. Furthermore, even some heavy editors have trouble correctly highlighting complicated command substitutions, whereas functions are generally perfectly fine.

By the way, I don't really understand why this issue's title mentions comments since it seems it's the command substitution that is incorrectly parsed.

@Rychu-Pawel
Copy link
Author

Nice work @alice-mm. I can confirm that changes from your current PR fixes my issue. I'll wait for it to get merged. Thanks!

@alice-mm
Copy link
Contributor

alice-mm commented Jul 9, 2018

You're welcome, @Rychu-Pawel. Also, grep -E '(foo|bar|plop)' is equivalent to grep -E 'foo|bar|plop', so you should be able to remove these problematic parentheses (from the (DEGRADED|FAULTED|… part) altogether:

$ seq 100 | grep -E '56|97'
56
97

(egrep has been deprecated for years, hence the grep -E in these examples.)

@RunDevelopment
Copy link
Member

Resolved by #1443.

@rmccullagh
Copy link

So is this change pushed to NPM? How do I get this change?

@RunDevelopment
Copy link
Member

@rmccullagh A new Prism version will be published on npm soon.
In the meantime, you can always get the very latest version of Prism at the download page.

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

No branches or pull requests

5 participants