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

(GH-1437) Fix detecting contiguous comment blocks and regions #1438

Merged

Conversation

glennsarti
Copy link
Contributor

PR Summary

Previously the region comment detection used a little convoluted method to
detect regions in a document. This commit simplifies the detection by
extracting the line from the document and using regex's similar to that used by
the PowerShell language configuration. This also removes the need for the
emptyline and subsequentText method calls. While the performance of the folder
is pretty quick, this should in theory make it faster on larger documents by
doing less calls to the VSCode Document API.


Previously the syntax folding feature would not correctly identify comment
blocks and comment regions if they appeared all together. This commit changes
the comment block detection to ignore line comments that start with region and
endregion, i.e. region block start/end directives. This commit also adds test
for this scenario.

Fixes #1437

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Previously the region comment detection used a little convoluted method to
detect regions in a document.  This commit simplifies the detection by
extracting the line from the document and using regex's similar to that used by
the PowerShell language configuration.  This also removes the need for the
emptyline and subsequentText method calls.  While the performance of the folder
is pretty quick, this should in theory make it faster on larger documents by
doing less calls to the VSCode Document API.
Previously the syntax folding feature would not correctly identify comment
blocks and comment regions if they appeared all together.  This commit changes
the comment block detection to ignore line comments that start with region and
endregion, i.e. region block start/end directives.  This commit also adds test
for this scenario.
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great Glenn!

Copy link
Collaborator

@rjmholt rjmholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@TylerLeonhardt TylerLeonhardt merged commit feaaa4d into PowerShell:master Jul 18, 2018
@cadayton
Copy link

cadayton commented Aug 6, 2018

The following was folding previously but not since the extension assume the folding logic.

# Initializing Rountine
   <block of code>
#

@glennsarti
Copy link
Contributor Author

@cadayton It would be better if you raised an issue (https://github.com/PowerShell/vscode-powershell/issues/new/choose) instead of commenting on this PR.

@TylerLeonhardt
Copy link
Member

@cadayton this is because we've switched away from indentation-based folding to syntax-based folding.

A work around for you would be to use #region Initialize Routine and #endregion. This is recommended.

Alternatively, you can simply disable syntax-based folding which will fall back to indentation-based folding. (not at my computer at the moment so I don't have the exact name but just search for "folding" in the settings.

I'm not sure we really want to support syntax-based folding and indentation-based folding at the same time so I would really encourage you to do one of the above workarounds.

@TylerLeonhardt
Copy link
Member

Like Glenn said, feel free to open an issue if you feel otherwise! 👍

@cadayton
Copy link

cadayton commented Aug 12, 2018 via email

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.

Syntax folding should differentiate between regions and comment blocks
4 participants