Feature/upgrade powershell #1213

aaroneg · 2019-06-21T01:59:00Z

This should fix or make moot the following issues:
closes #640
closes #966
closes #1041
closes #1208
closes #1209
closes #1210

This should make it unnecessary to merge:
closes #967
I would like to thank @miparnisari for caring enough to open a PR ❤

It does NOT fix, but partially addresses:
#1025 , and it would be nice to get some more expert help on this one.

It doesn't handle two types declarations on the same line for some reason, and perhaps someone more accomplished with rouge can help me figure out why. Perhaps @jneen ? ( See the first few lines of the 'Add-Extension' function in the sample output )

It also adds support for comment based help ( https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_comment_based_help?view=powershell-6 )

fixes #1209

expand types, simplify documentation / multiline keywords, fix heredocs add to visual samples

#1025 #1041

# #640

aaroneg · 2019-06-21T03:33:37Z

( And thanks to @gmckeown for #660 )

ashmaroli · 2019-06-21T03:40:23Z

@aaroneg The comment closes #1210 has to be within the opening post for GitHub to actually close the issue when this merges..

aaroneg · 2019-06-21T03:45:44Z

Ah thanks @ashmaroli

pyrmont

Let me know if you have any questions about the comments!

lib/rouge/lexers/powershell.rb

gmckeown · 2019-06-21T14:37:00Z

~~@aaroneg Is the removal of built-ins highlighting deliberate?~~

Ignore, as I figured out what you were doing here now.

lib/rouge/lexers/powershell.rb

spec/visual/samples/powershell

sure. Co-Authored-By: Ashwin Maroli <ashmaroli@users.noreply.github.com>

… into feature/upgrade-powershell

pyrmont · 2019-06-26T22:06:02Z

@gmckeown Sorry, bringing the discussion back to the main PR since we've resolved the other stuff. I discovered that a problem with my lexing logic was the parsing of attributes. I've since fixed this but do not think the list in ATTRIBUTES is complete. It doesn't cause any errors since the lexer will just assume anything it doesn't recognise as an attribute is a type but it's a little incorrect. What do you think? I'm honestly at this point inclined just to leave it as-is and wait for people to complain before fixing it.

@jpmarceau Sorry to drag you into this but since I now know you use the PowerShell highlighting, too, do you have any thoughts?

gmckeown · 2019-06-26T23:13:25Z

I ... do not think the list in ATTRIBUTES is complete. It doesn't cause any errors since the lexer will just assume anything it doesn't recognise as an attribute is a type but it's a little incorrect. What do you think? I'm honestly at this point inclined just to leave it as-is and wait for people to complain before fixing it.

It does contain all of the cmdlet binding attributes, so I guess the question is whether you're expecting other attributes to be included?

Does parameter belong in here? VSCode, for example, treats parameter more like a type and certainly differently to a cmdlet binding attribute:

It is formally considered an attribute, but should we distinguish between cmdlet binding attributes and function parameter attributes? They're certainly not interchangeable -- you can't put parameter inside cmdletBinding and you can't put confirmImpact as an attribute of a function parameter.

pyrmont · 2019-06-26T23:16:12Z

@gmckeown We also have CmdLetBinding as an attribute, though. Should it and Parameter both be taken out?

gmckeown · 2019-06-26T23:19:10Z

We also have CmdLetBinding as an attribute, though. Should it and Parameter both be taken out?

I think yes. Did @aaroneg include CmdletBinding in there initially? Maybe there was a reason?

gmckeown · 2019-06-26T23:22:23Z

lib/rouge/lexers/powershell.rb

+        rule %r/(function)(\s+)(?:(\w+)(:))?(\w[-\w]+)/i do
+          groups Keyword::Reserved, Text::Whitespace, Name::Namespace, Punctuation, Name::Function
+        end
+        rule %r/(?:#{AUTO_VARS})\b/i, Name::Builtin::Pseudo


@pyrmont I just want to re-iterate that I think this is an over-simplistic way of identifying the automatic variables -- I did it as a quick test locally, but it's not going to correctly highlight them in strings and possibly in other contexts.

Ah. Good to know. I should probably move the test to the :variable state.

pyrmont · 2019-06-26T23:22:33Z

@gmckeown It was in there from the beginning (@aaroneg took it out in the initial commit but he later put it back). I think we can take it out and highlight it like VSCode does.

gmckeown · 2019-06-26T23:26:27Z

I think we can it and highlight it like VSCode does.

That works for me!

pyrmont · 2019-06-26T23:37:32Z

@gmckeown I'll see if @jpmarceau has any thoughts. He very quickly noticed an error in the PowerShell lexer in 3.5.0 so might have a helpful perspective. It's looking pretty close to merging to me.

gmckeown · 2019-06-27T00:06:55Z

@pyrmont Just spotted an oddity in the visual sample with the latest lexer:

Not sure why Position is a Name.Builtin.Pseudo when the other items are Keyword.Type. Is it this rule in bracket handling:

       rule %r/([A-Za-z]\w+)/ do |m|
          if ATTRIBUTES.include? m[0]
            token Name::Builtin::Pseudo
          else
            token Keyword::Type
          end
        end

I guess include? is doing a partial match and is picking up PositionalBinding?

pyrmont · 2019-06-27T00:12:42Z

@gmckeown I'm away from my computer but I'm not sure. I'll have a look later this evening!

pyrmont · 2019-06-27T01:06:04Z

@gmckeown I'm a moron. I was mashing the elements in the ATTRIBUTES array together but I don't want to do that any more given how it's being used now. I've fixed and that's solved the problem. And you were right: the way String#include? works was causing it to do a partial match.

gmckeown · 2019-06-27T10:03:10Z

lib/rouge/lexers/powershell.rb

@@ -14,8 +14,8 @@ class Powershell < RegexLexer

      # https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_commonparameters?view=powershell-6


For accuracy, should now change this comment to:

# https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_functions_cmdletbindingattribute?view=powershell-6

Have made the change (only found out that GitHub allows a direct edit and commit from the UI...).

jpmarceau · 2019-06-27T12:37:56Z

@pyrmont thanks for asking but sadly I can't really offer much input here. I quickly spotted the issue with 3.5.0 because it was causing my deployment to fail, but other than that I don't have enough expertise with your project to be of much use.

pyrmont · 2019-06-28T00:49:58Z

@jpmarceau No problem! Just wanted to check :)

pyrmont · 2019-06-28T01:50:47Z

Thanks to everyone that contributed as part of this PR. We have a far more robust lexer as a result :)

If you spot any problems, please always feel free to file an issue!

aaroneg added 11 commits June 20, 2019 14:47

Starting over.

783e4ba

Better examples

e606f67

A bit less drastic.. just cut out the builtins

9e55010

This line is not necessary.

152cf31

Embiggen the sample

df31cf4

for testing #1208

f79e6ca

fixes #1208

1439aaf

fixes #1209

Documentation references

39e3366

Fixes.

1164706

expand types, simplify documentation / multiline keywords, fix heredocs add to visual samples

Tests for

4dac128

#1025 #1041

Tests

b8e4ec4

# #640

pyrmont added the needs-review The PR needs to be reviewed label Jun 21, 2019

pyrmont suggested changes Jun 21, 2019

View reviewed changes

pyrmont added author-action The PR has been reviewed but action by the author is needed and removed needs-review The PR needs to be reviewed labels Jun 21, 2019

pyrmont mentioned this pull request Jun 21, 2019

Switch from explicit built-ins to verb-noun regex #1210

Closed

aaroneg added 3 commits June 21, 2019 07:49

Remove unnecesary commas

edbf1c6

path test

2763d83

regex fixes from reviewers

0584687

ashmaroli reviewed Jun 21, 2019

View reviewed changes

lib/rouge/lexers/powershell.rb Outdated Show resolved Hide resolved

lib/rouge/lexers/powershell.rb Outdated Show resolved Hide resolved

spec/visual/samples/powershell Outdated Show resolved Hide resolved

aaroneg and others added 6 commits June 21, 2019 08:18

extra commas in regex

af228d5

.

f9b7a0e

Swap 0-9 for \d, drop generic

89cdb55

Update lib/rouge/lexers/powershell.rb

329a184

sure. Co-Authored-By: Ashwin Maroli <ashmaroli@users.noreply.github.com>

Fix regex

3f0e137

Merge branch 'feature/upgrade-powershell' of github.com:aaroneg/rouge…

6d102fc

… into feature/upgrade-powershell

Fix incorrect merge, update rules

8649366

pyrmont approved these changes Jun 26, 2019

View reviewed changes

pyrmont added 2 commits June 27, 2019 06:19

Merge branch 'master' into feature/upgrade-powershell

d03bf03

Fix lexing of attributes

0fff498

Fix RuboCop warning

548f54a

gmckeown reviewed Jun 26, 2019

View reviewed changes

Update attributes, move automatic variable rule

21f0528

Leave attributes as array

65593e2

pyrmont added needs-review The PR needs to be reviewed and removed author-action The PR has been reviewed but action by the author is needed labels Jun 27, 2019

gmckeown reviewed Jun 27, 2019

View reviewed changes

Amend URL in comment for cmdlet binding attributes

fdfe9d8

pyrmont merged commit f1125f3 into rouge-ruby:master Jun 28, 2019

aaroneg deleted the feature/upgrade-powershell branch June 28, 2019 01:52

This was referenced Jun 28, 2019

PowerShell Highlighter miscategorises typecasts and braced variables #1025

Closed

Windows PowerShell language highlighting faulty #1041

Closed

pyrmont removed the needs-review The PR needs to be reviewed label Jun 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/upgrade powershell #1213

Feature/upgrade powershell #1213

aaroneg commented Jun 21, 2019 •

edited

Loading

aaroneg commented Jun 21, 2019

ashmaroli commented Jun 21, 2019

aaroneg commented Jun 21, 2019

pyrmont left a comment

gmckeown commented Jun 21, 2019 •

edited

Loading

pyrmont commented Jun 26, 2019

gmckeown commented Jun 26, 2019

pyrmont commented Jun 26, 2019 •

edited

Loading

gmckeown commented Jun 26, 2019 •

edited

Loading

gmckeown Jun 26, 2019

pyrmont Jun 26, 2019

pyrmont commented Jun 26, 2019 •

edited

Loading

gmckeown commented Jun 26, 2019

pyrmont commented Jun 26, 2019

gmckeown commented Jun 27, 2019

pyrmont commented Jun 27, 2019 •

edited

Loading

pyrmont commented Jun 27, 2019

gmckeown Jun 27, 2019

gmckeown Jun 27, 2019

jpmarceau commented Jun 27, 2019

pyrmont commented Jun 28, 2019

pyrmont commented Jun 28, 2019

		@@ -14,8 +14,8 @@ class Powershell < RegexLexer

		# https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_commonparameters?view=powershell-6

Feature/upgrade powershell #1213

Feature/upgrade powershell #1213

Conversation

aaroneg commented Jun 21, 2019 • edited Loading

aaroneg commented Jun 21, 2019

ashmaroli commented Jun 21, 2019

aaroneg commented Jun 21, 2019

pyrmont left a comment

Choose a reason for hiding this comment

gmckeown commented Jun 21, 2019 • edited Loading

pyrmont commented Jun 26, 2019

gmckeown commented Jun 26, 2019

pyrmont commented Jun 26, 2019 • edited Loading

gmckeown commented Jun 26, 2019 • edited Loading

gmckeown Jun 26, 2019

Choose a reason for hiding this comment

pyrmont Jun 26, 2019

Choose a reason for hiding this comment

pyrmont commented Jun 26, 2019 • edited Loading

gmckeown commented Jun 26, 2019

pyrmont commented Jun 26, 2019

gmckeown commented Jun 27, 2019

pyrmont commented Jun 27, 2019 • edited Loading

pyrmont commented Jun 27, 2019

gmckeown Jun 27, 2019

Choose a reason for hiding this comment

gmckeown Jun 27, 2019

Choose a reason for hiding this comment

jpmarceau commented Jun 27, 2019

pyrmont commented Jun 28, 2019

pyrmont commented Jun 28, 2019

aaroneg commented Jun 21, 2019 •

edited

Loading

gmckeown commented Jun 21, 2019 •

edited

Loading

pyrmont commented Jun 26, 2019 •

edited

Loading

gmckeown commented Jun 26, 2019 •

edited

Loading

pyrmont commented Jun 26, 2019 •

edited

Loading

pyrmont commented Jun 27, 2019 •

edited

Loading