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) adds NSIS v3.08 commands, some small improvements #3351

Merged
merged 10 commits into from Oct 13, 2021
Merged

enh(nsis) adds NSIS v3.08 commands, some small improvements #3351

merged 10 commits into from Oct 13, 2021

Conversation

idleberg
Copy link
Contributor

@idleberg idleberg commented Oct 6, 2021

Changes

Adds NSIS v3.08 commands
Fixes case sensitivity option

Checklist

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

@joshgoebel
Copy link
Member

joshgoebel commented Oct 6, 2021

I changed the constants to variable.constant. I wonder if all the many "keywords" should become built_in or function.name... they look more like utility functions than proper keywords.

PARAM_NAMES is a little confusing also... do any of the newer scopes we're using apply perhaps?

@joshgoebel
Copy link
Member

Also, what is nsExec::ExecToLog "calc.exe" here exactly? class as a scope has been deprecated, so we need a new scope.

Screen Shot 2021-10-06 at 7 22 57 PM

@idleberg
Copy link
Contributor Author

idleberg commented Oct 6, 2021

Also, what is nsExec::ExecToLog "calc.exe" here exactly?

In NSIS terms, it's a command (ExecToLog) provided by a plugin (nsExec). While most plugins are third-party projects that can be installed by users of NSIS, there are several first-party plugins bundled with NSIS (AdvSplash, Banner, BgImage, Dialer, InstallOptions, LangDLL, Math, nsDialogs, nsExec, NSISdl, Splash, StartMenu, System, TypeLib, UserInfo and VPatch). I hope this helps to find a better classification for plugin commands.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 7, 2021

In NSIS terms, it's a command (ExecToLog) provided by a plugin (nsExec).

I'm thinking:

<title.function>nsExec::ExecToLog</>

or

<title.class>nsExec</>::<title.function>ExecToLog</>

Thoughts? title.function is most similar to what we were doing before and obviously the latter draw more of a distinction between the module name and the functions it provides.

@joshgoebel
Copy link
Member

@idleberg That was an either/or question. :)

@idleberg
Copy link
Contributor Author

idleberg commented Oct 7, 2021

You probably have a better overview if there are similar cases in other languages and can make a better decision on how these cases should be treated. Semantically, the latter proposal makes sense to me. On the other hand, there are no use cases for a class alone, one can only use the combination of a class and a method (class::function). So maybe there is no practical sense to use different scopes. Personally, I have no preference really. It would be nice to get another opinion on this, but I doubt the interest in NSIS is high enough to get one.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 7, 2021

Personally, I have no preference really.

Fair enough... but ask yourself: Would you find one or the other easier to read in a highlighted code snippet? That's kind of what I was getting at.

@allejo Any thoughts?

@allejo
Copy link
Member

allejo commented Oct 12, 2021

Hello 👋

Coming from other programming languages, having a differentiation between a class name and a function is helpful in terms of highlighting but that's because they each carry a separate meaning on their own.

Based solely on this conversation, it sounds to me that since you can't have a class by itself in NSIS then highlighting them separately wouldn't really carry any useful meaning. To me, this behavior sounds a lot more like a namespace rather than a class, like you'd see in C++ with std::string and std::string::npos; I'd expect both of these values to be highlighted as a single value instead of std being a "class."

My vote would be to highlight it as,

<title.function>nsExec::ExecToLog</>

@joshgoebel
Copy link
Member

Added highlighting for Function blah and Function .blah style patterns back. I think this is good to go now.

@joshgoebel
Copy link
Member

@idleberg Thanks so much. This will land in 11.3, hopefully this week.

@joshgoebel joshgoebel changed the title Add GetWinVer enh(ssis) Adds NSIS v3.08 commands Oct 13, 2021
@joshgoebel joshgoebel changed the title enh(ssis) Adds NSIS v3.08 commands enh(nsis) adds NSIS v3.08 commands, some small improvements Oct 13, 2021
@joshgoebel joshgoebel merged commit 7cb4e3d into highlightjs:main Oct 13, 2021
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.

None yet

3 participants