goolord / alpha-nvim

a lua powered greeter like vim-startify / dashboard-nvim
MIT License
1.84k stars 109 forks source link

refactor: avoid using unnecessary globals #62

Closed kylo252 closed 2 years ago

kylo252 commented 2 years ago

Convert globals to native lua modules which are both more performant and generally encouraged.

goolord commented 2 years ago

have you tested this? i'm 90% sure this doesn't work, everything that's global is inherently stateful and gets reset on each require

kylo252 commented 2 years ago

have you tested this?

It does work as usual for normal operations, or what did you have in mind?

https://user-images.githubusercontent.com/59826753/145669266-41e0e969-170f-44cc-9d53-f0b62f5041e7.mp4

and gets reset on each require

It's kind of the opposite, you need to do an extra step in order to remove the caching of require in lua.

https://github.com/nanotee/nvim-lua-guide#reloading-cached-modules

goolord commented 2 years ago

i am well aware. in fact my first instinct was to avoid using globals, but i ran into problems with some of the stateful behavior like redrawing and keymaps, but testing your pr everything works ok so i guess it will remain a mystery :9

kylo252 commented 2 years ago

You can go a step further and create a metatable for it. I can show you an example of how that would look like if you'd like.

goolord commented 2 years ago

well, i'm curious what problem a metatable would solve

kylo252 commented 2 years ago

well, i'm curious what problem a metatable would solve

It's used for "state" management by using an OOB model.


I didn't want to rename the function originally just to make reviewing easier, but I'd suggest using some more descriptive names, especially since these don't take any arguments.

press() ---> on_press() set_cursor() ----> on_cursor_moved()

goolord commented 2 years ago

to me, on_x implies that a closure should be passed as an argument. maybe something like perform_press and move_cursor ?

kylo252 commented 2 years ago

to me, on_x implies that a closure should be passed as an argument. maybe something like perform_press and move_cursor ?

Good point! I like move_cursor, but perform_press is still vague. It should be something like:

You get the point 😄

goolord commented 2 years ago

https://github.com/goolord/alpha-nvim/pull/62/commits/dec6d9d756dfc7ba1e1921ae4e5c30cb69b6be8e should be reverted, the layout api is intentionally extensible

kylo252 commented 2 years ago

dec6d9d should be reverted, the layout api is intentionally extensible

It's just that they're very specific to the internals that they aren't exactly that useful, not even for changing the current behavior, since you're always calling the local function. Maybe convert some/all of them to be called as M.func(...)?

kylo252 commented 2 years ago

I kept the renames to minimal just to avoid introducing breaking changes. Feel free to rename the functions if you'd like.

Otherwise, I think that this PR has gotten big enough as is, so it's better to break out further changes.

goolord commented 2 years ago

cool, thanks for the contribution ❤️

goolord commented 2 years ago

ok now that i'm a little more awake i'm able to reason about things a little better

layout_element and keymaps_element are actually both tables, not functions. lua actually only carries around 1 copy of the table and any operations you do on it will mutate that table. i think tables and maybe functions are the only types that work like this. the section pattern for each theme works on the same principle

kylo252 commented 2 years ago

layout_element and keymaps_element are actually both tables, not functions.

This distinction doesn't really exist in lua

Functions are values Notice that in the above example, we didn't actually "name" the function, we just assigned it to a variable. This is because in Lua functions are regular values (like numbers, strings, tables, etc.), and you can do anything with them that you can do with any other value. This is very different from many other languages that you might know (like C), where functions have permanent names fixed at compile-time, and can't be manipulated like values. ^1

goolord commented 2 years ago

function and table are both primitive types, they are treated differently by the compiler http://lua-users.org/wiki/ImmutableObjects

Lua tables are mutable

Lua local variables are mutable (in terms of the association between variable and value, not the value itself), while global variables are implemented as tables and so share the same conditions as tables

Functions can store data in upvalues, environment variables, or in storage areas accessible through other function calls (e.g. databases). A function can be considered immutable when it is pure [1].

i understand your impulse to be helpful but i am not a noob 😅 i read a lot of documentation and papers about lua performance before i wrote this plugin

the conditions under which you could consider a function as having 'mutated' are little fuzzy to me, but it doesn't rly matter since layout_element and keymaps_element are both tables and thus both mutable