returned an error each time something bad occurred. #190
Conversation
Signed-off-by: Augustin Husson <[email protected]>
|
ah that's a weird error . Interesting ... It's returns now the following error :
It seems that the code returned doesn't match the error message. |
| location, err := s.cache.Find(¶ms.TextDocumentPositionParams) | ||
| if err != nil { | ||
| return nil, nil | ||
| return nil, err |
There was a problem hiding this comment.
That's incorrect.
See #185 for why.
There was a problem hiding this comment.
I still don't get why we couldn't return an error saying the document is not there. In the point of view of the cache that's an error.
It should be on the side of the caller to decide what to do with it using a sort of switch case on the error.
There was a problem hiding this comment.
We shouldn't suppress errors about documents being not there. In fact this error shouldn't have happened in the first place.
The only type of error that should be suppressed is, if the document context has expired, since in that case the request is silently cancelled to avoid returning outdated results to the client.
There was a problem hiding this comment.
For explanation: An expired document context means, the document content has changed while processing the request. While that's rare, it shouldn't result in an error. However in that case, an error is raised internally, since the request can't be handled in a sane way. These special errors have to be filtered out, before responding to the client.
How did you get that error? For the example from the issue, I now get the correct response: The error code is a |
|
Btw, you can ignore the test failure raised by the CI for now. It will be fixed as part of #181 . |
ah sorry, I was talking about the error in the CI
Well yes since I'm returning systematically an error if the document is empty, then it fixed the current issue. |
| } | ||
|
|
||
| call, ok := location.Node.(*promql.Call) | ||
| if !ok { | ||
| return nil, nil | ||
| return nil, errors.New("no node find for the given query") |
There was a problem hiding this comment.
This code branch is not an error condition. In case we reach this, the cursor is not over a function call and we simply return an empty result.
Also note, that returning nil pointers isn't a problem with this endpoint, since no member accesses take place.
A switch case would make to many assumptions about the IMO, the easiest option would be to only return |
|
Can we split this into two PRs?
|
|
sure no issue. Even more, if you want to do it on yourside, we can decline this PR. I don't mind at all ;). Doesn't look like it will be a quick fix so for sure I won't have the time to do it before some times. But maybe it's just me that will certainly go a bit too far hahaha. |
|
I'm declining this PR for the following reason:
|
fix #189
now it returns an http error 500.
I suppose we should type the error coming from the cache to associate them to a better HTTP error code