pappasam / nginx-language-server

Language server for nginx.conf
GNU General Public License v3.0
60 stars 2 forks source link

Some directives in `if` block does not get a hover #10

Closed harry-xm closed 3 years ago

harry-xm commented 3 years ago

For example, the limit_rate directive has Contexts: http, server, location, ifinlocation, But hover does not work in this if block.

I'm using the builtin LSP client from Neovim 0.5, but this should apply to other clients too.

http {
    server {
        server_name example.com;

        set $ups bing.com;

        location / {
            if ($request_method = POST) {
                limit_rate 10k;
            }
            proxy_pass http://$ups;
        }
    }
}
harry-xm commented 3 years ago

Quick patch:

diff --git a/nginx_language_server/server.py b/nginx_language_server/server.py
index 8e80fe3..28bce82 100644
--- a/nginx_language_server/server.py
+++ b/nginx_language_server/server.py
@@ -91,6 +91,8 @@ def hover(
     if not line:
         return None
     contexts = line.contexts if line.contexts else ["main"]
+    if len(contexts) >= 2 and contexts[-1] == "if" and contexts[-2] == "location":
+        contexts = contexts[:-1] + ["ifinlocation"]
     last_context = contexts[-1]
     possible_directives = (
         DIRECTIVES[last_context] if last_context in DIRECTIVES else {}

Edit: take 2 (less memory allocations)

diff --git a/nginx_language_server/server.py b/nginx_language_server/server.py
index 8e80fe3..dddbfe1 100644
--- a/nginx_language_server/server.py
+++ b/nginx_language_server/server.py
@@ -92,6 +92,8 @@ def hover(
         return None
     contexts = line.contexts if line.contexts else ["main"]
     last_context = contexts[-1]
+    if len(contexts) >= 2 and contexts[-1] == "if" and contexts[-2] == "location":
+        last_context = "ifinlocation"
     possible_directives = (
         DIRECTIVES[last_context] if last_context in DIRECTIVES else {}
     )
pappasam commented 3 years ago

Thanks for the patch! Would you be able to explain how / why it works? Happy to include it but I don't fully understand how the "ifinlocation" context works. For example, could if be in something else and called "ifinsomethingelse"?

harry-xm commented 3 years ago

ifinlocation came from download_hint_data.js, which turned if in location into ifinlocation with removeBlank().

To make it unambiguous we could give contexts a better data structure and parse x in y properly, but that would require more coding.

https://github.com/pappasam/nginx-language-server/blob/3501846d0e34da294029b69339b1fe7c53baa314/utils/download_hint_data.js#L30

https://github.com/pappasam/nginx-language-server/blob/3501846d0e34da294029b69339b1fe7c53baa314/utils/download_hint_data.js#L180

pappasam commented 3 years ago

@harry-xiaomi after some thought, I like your solution. Would you be able to submit this as a PR?

harry-xm commented 3 years ago

Just discovered that this would affect autocomplete of return. Context server, location, if would not match ifinlocation.

harry-xm commented 3 years ago

PR done with extra tweaking. No completion support though.