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.

Sign up for GitHub

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

Jump to bottom

Convert Code Folding from Tokens to use the AST (Target v2.0) #813

Closed
glennsarti opened this issue Dec 9, 2018 · 3 comments
Closed

Convert Code Folding from Tokens to use the AST (Target v2.0) #813

glennsarti opened this issue Dec 9, 2018 · 3 comments
Labels
Area-Folding Issue-Enhancement A feature request (enhancement).

Comments

@glennsarti
Copy link
Contributor

The AST contains the most correct version of how a script is interpreted. This includes regions of text. Currently the code folder only uses the Tokens which requires the folder to re-implement some of the AST behaviour e.g. matching token pairs for arrays etc.

The code folder should be implemented using as much of the AST as possible and then some performance metrics to ensure that the using the AST doesn't take longer than with tokens only.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 9, 2018
@glennsarti
(PowerShellGH-813) Use AST for code folding
22ead9d 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 12, 2018
@glennsarti
(PowerShellGH-813) Use AST for code folding
b24e7c4 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 12, 2018
@glennsarti
(PowerShellGH-813) Use AST for code folding
ffe2485 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 12, 2018
@glennsarti
(PowerShellGH-813) Use AST for code folding
e33ba46 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
* Moved the code to modify the end line of folding regions to the language
  server code.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 13, 2018
@glennsarti
(PowerShellGH-813) Refactor the FoldingReference arrays into it's own…
e2dc02b 
… class

TODO ..  Basically stop creating intermediate arrays and just use an extended Hash object
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 13, 2018
@glennsarti
(PowerShellGH-813) Refactor the FoldingReference arrays into it's own…
0cd9671 
… class

TODO ..  Basically stop creating intermediate arrays and just use an extended Hash object
@SydneyhSmith SydneyhSmith added Area-Folding Issue-Enhancement A feature request (enhancement). labels Dec 13, 2018
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 14, 2018
@glennsarti
(PowerShellGH-813) Use AST for code folding
73b6030 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
* Moved the code to modify the end line of folding regions to the language
  server code.
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Dec 14, 2018
@glennsarti
(PowerShellGH-813) Refactor the FoldingReference arrays and lists int…
e4744ca 
…o it's own class

Previously the folding provider created many intermediate arrays and lists and
required post-processing.  This commit changes the behaviour to use an
accumlator patter with an extended Dictionary class.  This new class adds a
`SafeAdd` method to add FoldingRanges, which then has the logic to determine if
the range should indeed be added, for example, passing nulls or pre-existing
larger ranges.

By passing around this list using ByReference we can avoid creating many objects
which are just then thrown away.

This commit also moves the ShowLastLine code from the FoldingProvider into the
Language Server.  This reduces the number of array enumerations to one.
@glennsarti
Copy link
Contributor Author

Can't be done right now due to having to support PS3. Closing issue as won't do ;_(

@glennsarti glennsarti reopened this Dec 14, 2018
@glennsarti
Copy link
Contributor Author

Reopened and changing to target v2.0 of the extension only.

@glennsarti glennsarti changed the title Convert Code Folding from Tokens to use the AST Convert Code Folding from Tokens to use the AST (Target v2.0) Dec 14, 2018
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Jan 21, 2019
@glennsarti
(PowerShellGH-813) Use AST for code folding
67a89ef 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
* Moved the code to modify the end line of folding regions to the language
  server code.
* The test fixture changes were needed due to how the tokeniser and ast
  see the beginning of some regions.  Users will probably not notice.

Note that this requires a modern PowerShell version due to use of
ASTVisitor2 class.
@glennsarti glennsarti mentioned this issue Jan 21, 2019
Closed
glennsarti added a commit to glennsarti/PowerShellEditorServices that referenced this issue Jan 21, 2019
@glennsarti
(PowerShellGH-813) Use AST for code folding
f9c404a 
The AST contains the most correct version of how a script is interpreted. This
includes regions of text. Currently the code folder only uses the Tokens which
requires the folder to re-implement some of the AST behaviour e.g. matching
token pairs for arrays etc.  The code folder should be implemented using as much
of the AST as possible.  This commit;

* Moves most of the region detection to use the AST Extents and uses a new
  FindFoldsASTVisitor.
* Modifies the tests and language server to use the new method fold detection
  class.
* Moved the code to modify the end line of folding regions to the language
  server code.
* The test fixture changes were needed due to how the tokeniser and ast
  see the beginning of some regions.  Users will probably not notice.

Note that this requires a modern PowerShell version due to use of
ASTVisitor2 class.
@glennsarti
Copy link
Contributor Author

So it appears that the non-Windows platforms don't have the parsing methods available. However if using tokens it all works ok. I see this as a blocker and there's basically no point in using the AST right now. maybe in the future this changes? Until then, closing this PR and the associated issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-Folding Issue-Enhancement A feature request (enhancement).
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@glennsarti @SydneyhSmith