-
Notifications
You must be signed in to change notification settings - Fork 46
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
WIP Full statement syntaxing #156
base: main
Are you sure you want to change the base?
Conversation
There are a few issue remaining:
There may be more. There are probably ways to get unexpected results, but that listed above is what I see from the normal code, or code examples, I have here. UPDATE: |
A JSON version of the syntax is available at https://github.com/msftrncs/PowerShell.tmLanguage/tree/argumentmode_2ndtry, I produced this PList version from there, |
I was able to test this grammar on Atom, though only to confirm that VS Code does not behave the same with |
I finally have IF/ELSEIF/ELSE working the way they should. This was a huge lesson in handling the grammar. The lesson learned is to 'always march forward'. Retreating the grammar rules needs to occur in only the right conditions to ensure inner sections of rules cannot reapply to similar but unrelated constructs. In this case, the only sure way out of a rule (or an entire stack of rules) is at the end of a line. There are still quite a few things I think should be worked on to make the grammar right, and with this lesson it will be easier to do that now. |
From the screenshots you've sent I'd say it's looking good! |
I got a solution for the deviation in Atom worked out in fd37e90, dealing with chained statements (if/elseif/else or try/catch/finally or any statement that uses a statement block). |
Showing the result of the changes in the last commit: (Edit, 'master' on left, this PR on right, same theme, after the @Splat regression was fixed and improved) |
Guess I should have been looking at all the tests in the PowerShell repository. Evaluating them for some work I am doing in PowerShell/PowerShell, and I noticed that I am missing allowing full statements behind hashtable assignments, and so when I tweaked things to allow that, it shows a huge flaw in the |
4d22dad
to
dd1791b
Compare
I have rebased and squashed, as the commit history at this point is mostly clutter, the entire syntax file has been practically completely overhauled by this PR. There is still a lot to do, as I would still need to apply scope adjustments and fix tests to bring this up to par. |
I tested this today to see if and how it might fix issue #194 I created. In it's current state it fixes all of the problems I was seeing except two.
|
For 1, its a theme thing. For 2, I am not sure there is any reason to scope it, nor a clear scope to specify. Many editors already do something about links. |
@msftrncs For the issue about the theme not supporting entity.name.parameter should that be opened as an issue with VSCode or the theme directly if it has a separate repository? Asking as I am using the default theme that ships with VSCode which I would expect to support all all of the possible syntax highlighting tokens. Also do you have a suggestion for a dark theme that does support all (or at least more) of the syntax token highlighting? |
Looking at things closer, its looking like |
When I manually edited the sub-scope to |
This is amazing, the accuracy is almost as good as the real syntax highlighting the ISE provided (at least with "normal" powershell code that doesn't try to be hard to read). The only issue I have found is that functions that don't use the standard verb-noun format share the same scope as the function name in function definitions. I've "fixed" this by replacing the regex on this line: https://github.com/msftrncs/PowerShell.tmLanguage/blob/ed163f4c9a7247acb7ab3deb24865dfc9061b326/powershell.tmLanguage.json#L2433 with this: The only issue with this "fix" is that doesn't work work if the command is prefixed by a path like |
@MartinGC94, thanks for the feedback, and sorry for the very long delay. Myself, I prefer if all commands or functions scope the same, unless they are truly language standard (built-in) functions. Unfortunately in PowerShell, nothing is really standard, as everything can be replaced, so I left the original 'approved verb'-noun concept scoping as I've been trying to work on really technical aspects, such as operator scoping, because that is where this still differs the most from ISE or PSReadLine, even though its not obvious. I just realized this is occurring: The |
Hey, sorry, I'm definitely catching up here. Is this PR intended / ready to be merged? What are the risks with it? Clearly it seems to have a lot of benefits. FYI @rjmholt @SydneyhSmith |
Good to see you are back to working on this, I was a little afraid the semantic highlighting had made you drop this but like I said before the earlier implementation with my change was pretty much perfect to me. I'm not really sure I follow the logic in having different scopes for commands that follow the verb-noun pattern vs commands that don't. The point of syntax highlighting is to help me see how my text is interpreted by the language, right? So if PS thinks "ls" and "Get-ChildItem" is the same then why shouldn't my editor show me that? Even more confusingly if you use an unapproved verb it also gets a different color/scope "Get-ChildItem" VS "Validate-Item". |
@andschwa the goal of this PR was to fix up the Textmate grammar. I think the semantic highlighting has probably eschewed a lot of the usefulness of this (it was created before semantic highlighting was an option) but it's probably still worth merging. The main risk would be for users who are "used to" old stylings and the problem quirks and have not turned on semantic highlighting, or have done some very specific custom themes, will be affected and have their colors changed potentially, but they can always re-adjust them back "how they like them" only now with proper symbols. |
@JustinGrote This Textmate grammar is used beyond the VS Code PowerShell extension though (and if I'm understanding it right, we do use it but as a fallback, with semantic highlighting on top), most prominently by GitHub's highlighting of PowerShell files, so it seems to me we should maintain and improve it regardless of the introduction of semantic highlighting to the extension. @msftrncs Regarding:
While I've not been involved in the design or discussion to this point, I think I agree with your point here that syntax highlighting is intended to show the reader how the text is interpreted by the language, so I wouldn't want approved/unapproved verbs (and therefore aliases) being rendered with different colors. (At least not be default!) Is that something blocking this PR? |
@andschwa I agree on all counts. |
Honestly the approved verb command scoping is carried over from the current release. There are two different scopes at play here for commands:
While I will admit that there needs to be some scoping of something along the lines of Comparisons to ISE are slightly flawed. ISE is limited entirely to the tokenizing of PowerShell script by the PowerShell engine (the same as PSReadLine), and is not really representative of higher granularity offered by textmate language grammars. Further I believe that the tokenizing of function names in the There is definitely a need to consider the effects of this PR on the ISE theme, as I have given it no concern so far. |
@msftrncs Do you think you'll get back to this in the near future? If not, I might take a stab at learning textmate grammar and try to write my own syntax. -Edit: I took a look at it and figured it will take more time and effort than I'm willing to put into this. If nobody from the community steps up then I hope Microsoft as a company will because the current grammar is awful. |
This PR demonstrates a nearly complete conversion of the syntax rules for the purpose of full statement syntaxing. Again, this PR is for feedback, and not entirely intended to be accepted.
This would resolve issues such as #152 and #153, and provides a little more theme opportunity, as now function calls can be scoped.
In the above, you can see that
writeproperty
,writeXMLvalue
andwritevalue
are scoped as a function call. (orange in the VS CodeMonokai Dimmed
theme my theme is based on) The red text is exceptions that I have yet to handle, so there is some work left.