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

Expand variable highlights to support Bash parameter expansion #20

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KlfJoat
Copy link

@KlfJoat KlfJoat commented Mar 1, 2024

  • Add support for all of the existing Bats variables in ${var} form.
  • Add tests for each of the matches using various real-world examples.
  • Fix some existing tests

Hi

I prefer to use the ${var} form of variables in Bash. However, the current matches for this extension do not support that form except for counting variables for output variables, like ${#lines[@]}.

This PR is some trivial work to help support both the $var and ${var} forms in Bats files, plus a little cleanup of the match file.

This does introduce an intentional regression. Previously, the $ would be included in the support.variable.bats scope, and thus be given the same foreground style as the variable name. However, when expanding the matches to include the ${var} style, this lead to inconsistent styling of the braces, even when they were captured and given the support.variable.bats scope.

Similarly, I could find no way to collapse the matches down so the file had just 1 entry per variable list, without inconsistent styling of the braces. In this case, that would be open brace { red and closed brace } yellow on the same ${var}, somehow.

I have also updated the test.bats file, where I've added a pretty extensive "workout" of the syntax coloring. I'm using the color customizations recommended in the README.md file. Since the visual "tests" in this file were pass-only, I've kept with that convention. However, I did some failing visual tests of my own and they all failed properly.

Finally, there were a few tests that seemed to be missing or incorrect, and I corrected them.

* Add support for all of the existing Bats variables in ${var} form.
* Add tests for each of the matches using various real-world examples.
* Fix some existing tests
@jetmartin jetmartin added the enhancement New feature or request label Mar 2, 2024
@jetmartin jetmartin self-assigned this Mar 2, 2024
@jetmartin
Copy link
Collaborator

Hi,
I'm traveling for work, I do my best to take a look.

I'll see if I find an idea for the "intentional regression" while I'll review/test it.

Thank you for real life examples ! I'm not developing much those days, this is valuable.
I would be glad to also have the falling test, it's something I must consider to properly test it.
I frequently do it but not have committed them yet. (may could it be in a separate file, a falling one and a passing one ?)

Best

@jetmartin
Copy link
Collaborator

Hi,

For some reason I do not have permissions to run the bellow command.
git pull git@github.com:KlfJoat/bats-vscode.git bash-parameter-expansion-highlight-support

I've create a branch and apply the patch. lets see if it apply the PR merge when I push it back...
I have fiew plane hours, I try to play on it over the flight.

Keep you posted.

@jetmartin
Copy link
Collaborator

Hi @KlfJoat ,

I've pushed a KlfJoat-bash-parameter-expansion-highlight-support branch.
I've change a bit the way you manage the ending brace over the flight yesterday.
I've made some test this morning, it looks fine.
Please test with your real life examples and send me feedback before I merge.

I need to deep a bit on your "intentional regression" as I do not see impact on my tests. but I was more focus on the brace thing and I have not active projects using bats-core currently to made proper test.

Regards

@KlfJoat
Copy link
Author

KlfJoat commented Mar 4, 2024

Hi,

Thanks for reviewing my PR.

I checked and your edits to the regexes don't support syntax highlighting when the variable is involved in Bash Parameter Expansion. That's the test cases that look like...

${BATS_TEST_DIRNAME##*/}
${BATS_TEST_DIRNAME%/*}
${BATS_TEST_DIRNAME:1:2}

Parameter Expansion allows users to manipulate the variable in various ways upon use. In those 3 examples...

  1. Remove the longest matching pattern from the start, leaving me with the rightmost directory in $BATS_TEST_DIRNAME
  2. Remove the shortest matching pattern from the end, leaving me with the opposite of item 1.
  3. Return the 2 characters starting at the second char (0-indexed). It's not relevant with a directory variable, but I chose it as an example of a different style of Parameter Expansion that might be performed with $lines or $output or $BATS_VERSION or $BATS_FILE_EXTENSION

It doesn't change the syntax highlighting of the braces at all.

My PR
bats-vscode_36dd4ad

Your regex
bats-vscode_eed4014

@KlfJoat
Copy link
Author

KlfJoat commented Mar 4, 2024

For some reason I do not have permissions to run the bellow command.
git pull git@github.com:KlfJoat/bats-vscode.git bash-parameter-expansion-highlight-support

I'm not sure why. On my side it shows the repo as Public. I followed the standard GitHub PR workflow as best I can, but I don't use GitHub much for collaboration so I might have changed a setting somewhere else that affects it?

Sorry.

@jetmartin
Copy link
Collaborator

Hi,
No PB for the PR.
Thank you for the feedback, I did it offline and I totally missed that point.
I’ll think about it again when I’ve got some more time available.
I’ll keep you posted.

@KlfJoat
Copy link
Author

KlfJoat commented Mar 4, 2024

FYI, I did things like this to try to get the braces included, but they didn't work consistently.

            <dict>
                <key>match</key>
                <string>(\${(output|status|lines|stderr|stderr_lines))\b[^}]*(})</string>
                <key>captures</key>
                <dict>
                    <key>1</key>
                    <dict>
                        <key>name</key>
                        <string>support.variable.bats</string>
                    </dict>
                    <key>3</key>
                    <dict>
                        <key>name</key>
                        <string>support.variable.bats</string>
                    </dict>
                </dict>
            </dict>

This is the inconsistent styling I mentioned while trying to get the braces to be green. Despite the fact that the inspector says that the curly braces in every one of the examples is the same, and despite the fact that it says they're all colored green, they were not green on the screen.

bats-vscode_inconsistent

Personally, I think that having the variable name properly highlighted is the most important part. The dollar sign and braces are not desirable, IMO.

@jetmartin
Copy link
Collaborator

Hi,

I've manage some test on the train this morning.
I've find why the interpretation did not match as expected. VSC has manage some contraint to avoir issues ont the regex.

  1. First you need to have only one group. if you do ({whatever you want}) it works but if you do ({whatever})({you want}) only the first one is matching.

  2. Second the inconsistency on {} and [] colorisation are unable to be fixed, it's override somewhere.

  3. the {(\[.*.\])?} syntax looks more "clean" to me than the trick (even if it works) \b[^}]*(}).

I've just pushed another regex who take it into account.
There should be fine now.
I still have some minor inconsistencies wich are for some linked to some behaviors of the point 2 I was not able to clearly identify.
But it's looks pretty good. at least enough to be functional.

Screenshot 2024-03-19 at 09 15 46

Please review it with some "real" code if you can.

Best

@KlfJoat
Copy link
Author

KlfJoat commented Mar 19, 2024

Hi,

I'm afraid I don't understand what the problem is that you're trying to solve.

In my opinion, what we want to highlight green are the special Bats variables only. My PR does that.

Your latest change highlights as green the Bash Parameter Expansion parts, which is undesirable. It also highlights some array indexes as green, which is also undesirable.

I'm not saying my PR is perfect. But if I don't understand what your goal is, I can't help you to reach it.

@KlfJoat
Copy link
Author

KlfJoat commented Mar 20, 2024

For some reason I do not have permissions to run the bellow command. git pull git@github.com:KlfJoat/bats-vscode.git bash-parameter-expansion-highlight-support

I've create a branch and apply the patch. lets see if it apply the PR merge when I push it back... I have fiew plane hours, I try to play on it over the flight.

FYI, I just found an alternative way that could have helped you out with this, so I want to share it.

GitHub creates git refs for every pull request. So if you run git ls-remote (or, from my forked repo, git ls-remote upstream), you'll see something like...

$ git ls-remote upstream
b39b988bfbdd5c458569d5e2f13a9ba114359e78        HEAD
cbec2a0cbc527eb44e719d6b3c22c189e7eaab47        refs/heads/KlfJoat-bash-parameter-expansion-highlight-support
b39b988bfbdd5c458569d5e2f13a9ba114359e78        refs/heads/master
9446d8bde21edd9ca984b8e23719e233066864a5        refs/pull/1/head
44a5e8059036a085f7a56d2a3443012e5e8be5a8        refs/pull/11/head
1d80f6c0238e564efdbc542d462ac6f4e4dbacd6        refs/pull/13/head
50723695d7f3cdcaadebd3c3fc998fdd851ce368        refs/pull/14/head
a466e677a2d87e5aa52c66013599f1bbb8eb96b4        refs/pull/15/head
b4708ca4ab1a8dd91f0e8953357cd5efe0f8d714        refs/pull/16/head
a098a9260591ec9d49dc426d01e028cf83691cfa        refs/pull/17/head
36dd4ad9706afe8382696e9ad82c313b4dcb819d        refs/pull/20/head
bf5ce4d18af44ef76841da1d4ae9fde60e93f80a        refs/pull/20/merge
28103fb461453b21ea97f7c4ce8b8d462c666c9d        refs/pull/4/head
bef579d5e42f33a456c68ca998561b614dccf972        refs/pull/5/head
7c29bca80ade2dd16b5f726cd851786508d191e8        refs/pull/7/head
a7551d390b2c5c26b3219a1be80dbfe5a9c164a4        refs/pull/8/head
838d502c814b55f69fb651c5a45924449df19ff9        refs/pull/9/head
f13af68650981a1a0a34ace80ebca7e5f96f90cc        refs/tags/v0.1.0
7ac752bac635e270962ecf263c833d44eb720e6e        refs/tags/v0.1.0^{}
8e4fc101c68fc98bac1b8bd4123ccc6e14aa2049        refs/tags/v0.1.1
727b35b53447b69e65a803e0a4fa3ad5c396ea46        refs/tags/v0.1.10
1a01142c0d740e97b38a758532cd7e1e0b9ccfb7        refs/tags/v0.1.10^{}
8f67c5000b2162dc2d58d185f28c1fc603f33320        refs/tags/v0.1.2
b519a61294f24e2f178fd248161a62bc23fcb4c3        refs/tags/v0.1.2^{}
55a80d03335f1009700c52d6db358493d890b6a6        refs/tags/v0.1.3
234c6737b82e5855504f5f64c4769513a64819f5        refs/tags/v0.1.3^{}
962c4d6e5dd59ac1a33d77fc464ead17b8dbcfb9        refs/tags/v0.1.4
5b835c33264927d1c94637b88a6c0ea5cfd6011b        refs/tags/v0.1.4^{}
c3f3a74c81517a68e9bcf91bf41cb8151002531d        refs/tags/v0.1.5
e1debca2ec255fada7f754749030c144156bab49        refs/tags/v0.1.5^{}
5f4ddd7707c455c30ca4c9b56fe669a3c8810e84        refs/tags/v0.1.6
68bd1c52875a6886fd80486f37a49237c9367f03        refs/tags/v0.1.6^{}
75f67ecb5e1234715ade676410e5d5646eab1526        refs/tags/v0.1.7
773f248587d5247309069d507484163564091929        refs/tags/v0.1.7^{}
aef2f313aecd0e032e5237daf8a80b44af936829        refs/tags/v0.1.8
d6961f86ffacd48ee85e318e584c2a207b4a7f93        refs/tags/v0.1.8^{}

Then you can fetch the PR and create a local branch...

git fetch origin refs/pull/20/head:KlfJoat-bash-parameter-expansion-highlight-support

Then git switch KlfJoat-bash-parameter-expansion-highlight-support, do your thing, merge if desired, and finally git push KlfJoat-bash-parameter-expansion-highlight-support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants