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

enh(nsis) Update variables pattern #3416

Merged
merged 4 commits into from
Dec 6, 2021
Merged

enh(nsis) Update variables pattern #3416

merged 4 commits into from
Dec 6, 2021

Conversation

idleberg
Copy link
Contributor

@idleberg idleberg commented Dec 3, 2021

Updates pattern for NSIS variable to allow .-characters. The bundled MUI2 header has been making use of this for years.

Changes

Allow . in variables regex pattern

Checklist

  • Added markup tests, or they don't apply here because...
  • Updated the changelog at CHANGES.md

src/languages/nsis.js Outdated Show resolved Hide resolved
@joshgoebel
Copy link
Member

Also adding some markup tests for this change would be good.

@idleberg idleberg mentioned this pull request Dec 3, 2021
2 tasks
@joshgoebel
Copy link
Member

joshgoebel commented Dec 3, 2021

Please add test/markup/nsis/variables.txt and you can just borrow a snippet from that header if you like.

@idleberg
Copy link
Contributor Author

idleberg commented Dec 3, 2021

@joshgoebel I've committed two simple test cases. However, writing them revealed some bugs I'm not sure how to fix.

The following tests will fail because they're in conflict with existing rules:

Literals

Var Custom.Variable
StrCpy $Custom.Variable "Hello World"
MessageBox MB_OK "$$Custom.Variable is: $Custom.Variable"

This test will fail since custom is detected as a literal

<span class="hljs-keyword">Var</span> <span class="hljs-literal">Custom</span>.Variable<span class="hljs-keyword">StrCpy</span> <span class="hljs-variable">$Custom.Variable</span> <span class="hljs-string">&quot;Hello World&quot;</span>
<span class="hljs-keyword">MessageBox</span> <span class="hljs-params">MB_OK</span> <span class="hljs-string">&quot;<span class="hljs-meta">$$</span>Custom.Variable is: <span class="hljs-variable">$Custom.Variable</span>&quot;</span>

Numbers

StrCpy $0 "Hello World"
MessageBox MB_OK "$$0 is: $0"

Again, this will fail since 0 is detected as a number

<span class="hljs-keyword">StrCpy</span> $<span class="hljs-number">0</span> <span class="hljs-string">&quot;Hello World&quot;</span>
<span class="hljs-keyword">MessageBox</span> <span class="hljs-params">MB_OK</span> <span class="hljs-string">&quot;<span class="hljs-meta">$$</span>0 is: $0&quot;</span>

These bugs should have existed before this PR was opened.

@joshgoebel
Copy link
Member

joshgoebel commented Dec 3, 2021

I think I fixed your first two issues:

Screen Shot 2021-12-03 at 5 12 00 PM


For custom I think perhaps we need a special rule for Var?

Var [VARIABLE_NAME_RE]

We can do that with a multi-matcher now:

{
match: [ 
  /Var/,
  /\s+/,
  VARIABLE_NAME_RE
],
scope: { 1: "keyword", 3: "variable" }
}

If we wanted to highlight the definition as variable also (others we could simply match it but apply no scope)... thoughts? Is Var that simple in actual usage?

@idleberg
Copy link
Contributor Author

idleberg commented Dec 3, 2021

If we wanted to highlight the definition as variable also (others we could simply match it but apply no scope)... thoughts?

I think that would be awesome

Is Var that simple in actual usage?

There's an optional parameter for Var, so it's either one of the following:

# This can be declared at top-level only
Var MyVariable

# This can be declared at block-level only
Var /GLOBAL MyVariable

I don't think that highlight.js currently supports any parameters in the style of /\/\w+, but we should add them at some point and in a separate PR.

Discourse

In both cases the variable will be global! When you try to declare a variable with the former syntax at block-level, the compiler will throw an error:

Var: currently, only global variables can be defined.

I suspect that block-level variable have been on the TODO list but were never implemented.

@joshgoebel
Copy link
Member

joshgoebel commented Dec 3, 2021

In that case what is the scope of /GLOBAL? params?

Screen Shot 2021-12-03 at 6 18 43 PM

@idleberg
Copy link
Contributor Author

idleberg commented Dec 3, 2021

In that case what is the scope of /GLOBAL? params?

Screen Shot 2021-12-03 at 6 18 43 PM

Should be fine for now, we can add more specific one later (if that's desired)

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

Successfully merging this pull request may close these issues.

2 participants