Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

symbols: do not limit symbols found from config #492

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

andrewnester
Copy link
Contributor

limiting symbols found from config could lead to confusion when broad search term returns less results that more specific one.

Fixes #491

limiting symbols found from config could lead to confusion when broad search term returns less results that more specific one.

Fixes sourcegraph#491
@codecov-io
Copy link

codecov-io commented Jul 24, 2018

Codecov Report

Merging #492 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #492   +/-   ##
=======================================
  Coverage   83.08%   83.08%           
=======================================
  Files          15       15           
  Lines        2052     2052           
  Branches      420      420           
=======================================
  Hits         1705     1705           
  Misses        345      345           
  Partials        2        2
Impacted Files Coverage Δ
src/typescript-service.ts 85.13% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d107424...770ac41. Read the comment docs.

@felixfbecker
Copy link
Contributor

Without the limit, on a monorepo this can easily return 100k symbols, which just JSON-encoding and sending over the wire will take forever. What is the use case for looking at more than 100 symbols?

@andrewnester
Copy link
Contributor Author

andrewnester commented Jul 24, 2018

@felixbecker yeah, I could agree with this point, maybe it's better to have higher limit then no limit at all.

But as far as I understood "sending over wire" happening between LSP client and server, not between TS server and LSP server, am I right?
If yes, then workspaceSymbol method still has proper limit here

.take(params.symbol ? 1000 : 100)

As to use case I described it in this issue #491

@felixfbecker
Copy link
Contributor

Cool, if it still has a limit applied at the end and the TS limit implementation does nothing but slice the final array I am in favor of this change.

@felixfbecker felixfbecker merged commit 50df375 into sourcegraph:master Jul 24, 2018
@andrewnester
Copy link
Contributor Author

wow, that was quick :) thanks @felixfbecker !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants