nathom / filetype.nvim

A faster version of filetype.vim
560 stars 35 forks source link

fix: properly handle shebangs #53

Closed mohamed-abdelnour closed 2 years ago

mohamed-abdelnour commented 2 years ago

Hello, @nathom!

https://github.com/nathom/filetype.nvim/blob/ddef0faedfb561ca2af141a35ab6f256beba8b5a/lua/filetype/init.lua#L183-L185

As of ddef0faedfb561ca2af141a35ab6f256beba8b5a, the default shebangs defined in lua/filetype/mappings/function.lua are not being applied, and we could potentially be indexing nil if custom_map is nil. This (hopefully) fixes both problems.

Closes #51.

mohamed-abdelnour commented 2 years ago

I made an adjustment to 02e0163c7712318ee57f3f3e663e8ca57c7f94e9. function_maps and function_maps.shebang are both non-nil, so it would be redundant to check.

Output of git diff 02e0163..e234522:

diff --git a/lua/filetype/init.lua b/lua/filetype/init.lua
index 80ab3dc..525968b 100644
--- a/lua/filetype/init.lua
+++ b/lua/filetype/init.lua
@@ -193,7 +193,7 @@ function M.resolve()
     local shebang = analyze_shebang()
     if shebang then
         shebang = shebang_from_map(shebang, custom_map)
-            or shebang_from_map(shebang, function_maps)
+            or function_maps.shebang[shebang]
             or shebang
         set_filetype(shebang)
     end
luukvbaal commented 2 years ago

What about reducing it to something like:

if shebang then
    shebang = custom_map and custom_map.shebang and custom_map.shebang[shebang]
        or function_maps.shebang[shebang] or shebang
    set_filetype(shebang)
end

Unless you expect to reuse shebang_from_map() in other places in the future.

mohamed-abdelnour commented 2 years ago

Hello, @luukvbaal. I think this is a matter of preference; to me, having it as a separate function makes it easier to reason about and maintain, especially because of how odd logical operators are in Lua. :)

That being said, I don't mind implementing either solution; I just wanted to fix this quickly so that it doesn't linger too long on the main branch. So, how about we get @nathom's opinion on this and move forward with whichever solution gets picked?

Either way, I definitely feel like there's room for improvement here. We could try to initialise custom_map or try to abstract try_lookup and reuse it, for example. I'm not sure what would be an idiomatic way of doing this in Lua; again, I wanted to get this fixed first and consider refactoring later.

luukvbaal commented 2 years ago

Sound good to me.

Either way, I definitely feel like there's room for improvement here.

Yeah I instantiate my dictionary config keys as empty tables for that reason in my plugins (stabilize.nvim/nnn.nvim), avoiding unnecessary nil guards. I'm not an expert though and have no idea whether it's idiomatic either so take it for what you will.

nathom commented 2 years ago

I think this is fine for now. I'll probably make it so that all custom maps begin as {} so we don't have to deal with nil checks later.