Symbols Support #265
Symbols Support #265
Conversation
|
Just FYI there was some flakyness with the tests, they failed, and then passed after being re-run (windows in particular) |
Aye, I assume that's related to #206 I'm not sure why it usually appears on Windows, but my theory is that it may be just slower to execute in that environment and therefore more likely to hit that race condition. |
There was a problem hiding this comment.
Given the current limitations of the parser this LGTM and I'd be happy to merge it, but it also means that we should prioritize resolving #8 if we keep the waiting logic in.
| cfgBlock, err := p.ParseBlockFromTokens(block) | ||
| if err != nil { | ||
| p.logger.Printf("parsing config block failed: %s", err) | ||
| continue |
There was a problem hiding this comment.
Just curious what's the motivation here behind logging and not erroring out here?
There was a problem hiding this comment.
I did not want a file with blocks we don't support yet var, local, etc to return an error to the client, I rather just log and move on
There was a problem hiding this comment.
Gotcha, that makes sense - can you just document this reasoning in the form of an in-line comment above the log line?
| func (r *datasourceBlock) Type() string { | ||
| return r.Labels()[0].Value | ||
| func (d *datasourceBlock) Type() string { | ||
| return d.Labels()[0].Value |
|
|
||
| // TODO: block until it's available <-pf.ParserLoadingDone() | ||
| // requires https://github.com/hashicorp/terraform-ls/issues/8 | ||
| // TODO: replace crude wait/timeout loop |
There was a problem hiding this comment.
🤔 I first thought this is "dirty", but then I realized that it may represent just the right balance between waiting for too long or erroring prematurely. So with that in mind I don't think it's crude, I actually think that functionally this is a sensible and pragmatic solution we could replicate in other handlers too eventually.
The only things I'd be careful about is
(a) the timeout - 3 seconds feels quite long to me, I'd reduce it down to 2 seconds max
(b) how this positions us towards/against complying with LSP. As far as I understand this waiting mechanism greatly increases the chance of us providing outdated data, and so that may put more pressure on resolving #8 otherwise we're diverging from the LSP, which we shouldn't be. This is why I leaned more towards erroring than waiting in the completion handler.
There was a problem hiding this comment.
I can adjust this to max 2s. The reason this is needed is because the documentSymbol request happens right away once a file is opened/changed, in my local testing the parser was never ready right away and it errored everytime. I have never hit a max timeout either fwiw, I suspect the parser just needed a few hundred milliseconds.
There was a problem hiding this comment.
The reason this is needed is because the documentSymbol request happens right away once a file is opened
I see, I wasn't aware of that. In that case I assume it's actually desirable to even wait longer before responding? i.e. we can and should tolerate longer timeout.
I was just applying caution because I saw some clients setting hard timeout for responses for some methods - e.g. formatting in particular is expected to be done within a second or two in some clients's implementations, otherwise the request is just cancelled. I assume the caution is not applicable to all methods then.
So feel free to keep it at 3 seconds.
There was a problem hiding this comment.
Regardless though we really need to resolve #8
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
ghost
locked as resolved and limited conversation to collaborators
This provides initial support for symbols navigation. Only supports symbols within the active file, and only jumps to provider, resource, and datasource blocks.
Closes #263