New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

awesomerc.lua: moved to local variables #2199

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@spk

spk commented Feb 21, 2018

Hi, just moved variables to local instead of global in awesomerc.lua, cheers

@Elv13

If that gets merged, it will break some people config. Everybody who split their config and access modkey or terminal from a module will see their config break. It is not a lot of people, but definitely more than zero.

Honestly, it's too late for that kind of changes. Maybe for 5.0, but not for 4.3.

@spk

This comment has been minimized.

spk commented Feb 21, 2018

Look like you have this kind of setup :troll:
I can remove the local for terminal, editor, editor_cmd and modkey; I don't think lots of people need myawesomemenu, mymainmenu etc as global, wdyt ? if not feel free to close the PR

@actionless

This comment has been minimized.

Member

actionless commented Feb 22, 2018

if i recall correct vicious widgets were encouraging using globals too

so i think some users which were based their lua codestyle on them or rc.lua could use a lot of globals around in the config

@psychon

This comment has been minimized.

Member

psychon commented Feb 22, 2018

@Elv13 Well, people who split up their config, already have their own config, no? So, they would have their own rc.lua and likely would not just blindly copy over the default config. This is just the default config, so either people copied it and changing the default does not bother them, or they did not make a copy and nothing breaks again.
Or am I wrong here?

If this is merged, we should remove the luacheck exception for rc.lua that allows it to set global variables.

@Elv13

This comment has been minimized.

Member

Elv13 commented Feb 22, 2018

Fair enough. That leaves only the shared modules as potential points to worry about. A quick "github wide search" found ~10 of these. But most are not very important

Some are intended for dofile() instead of require so they are not as problematic.

https://github.com/esn89/searchPrompt

I still have some issues with modkey and clientkeys. As seen in

https://github.com/Elv13/tyrannical/blob/master/shortcut.lua#L135

modkey and clientkeys are quite required in modules that wants to add new keybindings automagically.

@psychon I will let you decide on this. It is going to break some people configs if they use their modules on top of the default rc.lua to autopatch Awesome, but as you said, that's not a lot of people. Then it will break a few modules, including 3 of mine. I prefer an harder stance on "don't break the API". Maybe "graduating" modkey and clientkeys into awful just as I did for layouts would be a good enough compromise as I could update my modules and use it and existing users would mostly not be affected because they use custom rc.lua

@psychon

This comment has been minimized.

Member

psychon commented Feb 23, 2018

@Elv13 How about making everything but modkey and clientkeys local then? A follow-up patch could then make this more explicit by replacing modkey = with _G.modkey = and AFAIK that would then allow to remove the luacheck exception, too (but I didn't check if this is really true).

@actionless

This comment has been minimized.

Member

actionless commented Feb 23, 2018

i like awful.key.modkey approach more

@Veratil

This comment has been minimized.

Contributor

Veratil commented Mar 1, 2018

What about tagging this for v5? Since this is a breaking change (and I'm all for semantic versioning, including what we're attempting to do moving forward) it should be slated for the next major version rather than minor.

@actionless actionless added this to the v5 milestone Mar 1, 2018

@actionless actionless referenced this pull request Nov 6, 2018

Closed

Create globalkeys early #2464

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment