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

Add support for maxItemsComputed #444

Merged
merged 2 commits into from
May 11, 2021

Conversation

metcalf
Copy link

@metcalf metcalf commented Apr 20, 2021

What does this PR do?

Adds support for a yaml.maxItemsComputed setting that mirrors json.maxItemsComputed from the built-in JSON language service. It caps the number of outline symbols and folding regions computed for performance reasons.

Companion PR: redhat-developer/vscode-yaml#486

What issues does this PR fix or reference?

redhat-developer/vscode-yaml#485

Is it tested? How?

Wrote automated tests.

Confirmed this fixes the bug:

  • Manually opened a very large YAML file with outline pane open as described in the issue.
  • Observed a notification that the limit was exceeded.
  • Did not observe large performance issues like I did before.

Confirmed settings update as expected:

  • Manually opened a smaller YAML file
  • Changed the configuration to a small value (50)
  • Observed the outline pane truncate to match the new configuration.

@coveralls
Copy link

coveralls commented Apr 20, 2021

Coverage Status

Coverage decreased (-0.3%) to 79.313% when pulling ba4c727 on metcalf:andrew-max-items into 4095b53 on redhat-developer:master.

@metcalf metcalf marked this pull request as ready for review April 20, 2021 18:11

return result.slice(0, context.rangeLimit - 1);
return result.slice(0, context.rangeLimit);
Copy link
Author

Choose a reason for hiding this comment

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

I believe this was an off-by-one error that I've now added a test for.

@@ -169,7 +185,17 @@ export class LanguageHandlers {
return;
}

return this.languageService.getFoldingRanges(textDocument, this.yamlSettings.capabilities.textDocument.foldingRange);
const capabilities = this.yamlSettings.capabilities.textDocument.foldingRange;
Copy link
Author

Choose a reason for hiding this comment

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

There weren't any existing tests for foldingRangeHandler so I didn't add any to test this new functionality (just the underlying getFoldingRanges).

@@ -275,5 +301,21 @@ describe('Document Symbols Tests', () => {
createExpectedDocumentSymbol('conditions', SymbolKind.Module, 6, 12, 10, 28, 6, 12, 6, 22, [root2])
);
});

it('Document symbols with a limit', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Why does this test and the one above on line 141 have the same description?

Copy link
Author

@metcalf metcalf Apr 20, 2021

Choose a reason for hiding this comment

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

Ah, sorry, I relied on the different describe blocks to differentiate them but I see this doesn't match the naming convention in the file. Will fix.
(misread the test file in my initial response)

These are for the hierarchical and non-hierarchical cases inside separate describe blocks.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks. I missed that in the truncated github diff view.

@evidolob evidolob merged commit 2bbd90c into redhat-developer:master May 11, 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

6 participants