After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749221 - Improve sh variable substitution highlighting
Improve sh variable substitution highlighting
Status: RESOLVED OBSOLETE
Product: gtksourceview
Classification: Platform
Component: Syntax files
3.16.x
Other Linux
: Normal enhancement
: ---
Assigned To: GTK Sourceview maintainers
GTK Sourceview maintainers
Depends on:
Blocks:
 
 
Reported: 2015-05-11 11:52 UTC by nullie
Modified: 2021-07-05 11:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Proposed patch (1.56 KB, patch)
2015-05-11 11:53 UTC, nullie
none Details | Review
Proposed patch, better context names, tests (2.67 KB, patch)
2015-05-14 05:42 UTC, nullie
committed Details | Review

Description nullie 2015-05-11 11:52:38 UTC
${x}y and $0x are highlighted as whole substitutions, while only ${x} and $0 should be highlighted.

I've tried to extend syntax with posix specification for handling braces and one-char variables.
Comment 1 nullie 2015-05-11 11:53:15 UTC
Created attachment 303207 [details] [review]
Proposed patch
Comment 2 Sébastien Wilmet 2015-05-13 12:36:36 UTC
Review of attachment 303207 [details] [review]:

It would be better to give more meaningful names to the two variable contexts, instead of 'variable1' and 'variable2'.

Can you provide examples to tests/syntax-highlighting/ ?

::: data/language-specs/sh.lang
@@ +193,3 @@
+        <!-- <context ref="arithmetic-expansion"/> -->
+        <!-- <context ref="command-substitution"/> -->
+        <!-- <context ref="variable-expansion"/> -->

If those commented contexts must not be included, it's better to remove them altogether. Or add a comment to explain why they are not included.
Comment 3 nullie 2015-05-13 12:39:57 UTC
These contexts should be included, but are not implemented yet.
Comment 4 Sébastien Wilmet 2015-05-13 12:50:00 UTC
Ah ok, so it was a work-in-progress. For the first patch it's better to not include them (even if they are commented). And then for a later patch you can add them (or having everything in the same patch, as you like).
Comment 5 nullie 2015-05-14 05:42:17 UTC
Created attachment 303349 [details] [review]
Proposed patch, better context names, tests
Comment 6 Sébastien Wilmet 2015-05-15 18:05:06 UTC
Review of attachment 303349 [details] [review]:

Looks good.
Comment 7 Sébastien Wilmet 2015-05-15 18:18:14 UTC
Comment on attachment 303349 [details] [review]
Proposed patch, better context names, tests

Pushed to the master branch:

https://git.gnome.org/browse/gtksourceview/commit/?id=835df2b3774c478847a7e06032ebc5caf4dfc7cb

(added the bug URL at the end of the commit)
Comment 8 GNOME Infrastructure Team 2021-07-05 11:02:09 UTC
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org.
As part of that, we are mass-closing older open tickets in bugzilla.gnome.org
which have not seen updates for a longer time (resources are unfortunately
quite limited so not every ticket can get handled).

If you can still reproduce the situation described in this ticket in a recent
and supported software version, then please follow
  https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines
and create a new ticket at
  https://gitlab.gnome.org/GNOME/gtksourceview/-/issues/

Thank you for your understanding and your help.