-
Notifications
You must be signed in to change notification settings - Fork 110
config: Call hotplug hooks in more places #1233
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
base: master
Are you sure you want to change the base?
Conversation
Also add an ACTION environment variable containing the same value as passed as a command-line argument. Some scripts presume they will only be called in the one existing place so they need to be updated also.
| local cf = io.open(name) | ||
| if not cf then | ||
| return false | ||
| else | ||
| cf:close() | ||
| return true | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also use fs.stat(filename, "type") == "reg" for this, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mnyaaaa.. good question. I looked at this myself. Initially the function was just moving around existing code. When I looked at the general consensus on how to do this in Lua, I found the following:
"Using plain Lua, the best you can do is see if a file can be opened for read, as per LHF."
-- https://stackoverflow.com/questions/4990990/check-if-a-file-exists-with-lua
I believe "LHF" here refers to Luiz Henrique de Figueiredo, the creator of Lua.
So this is both in line with the Lua way of doing things, and it's what the existing code already does so I left it as is. That said, I'm certainly open to changing it to be more POSIXey but I was trying to do things in a Lua way because my Lua experience is very limited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Both seems fine to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to switch to @pony1k suggested version as it is more compact and we already depend on those libraries
G10h4ck
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments
| local cf = io.open(name) | ||
| if not cf then | ||
| return false | ||
| else | ||
| cf:close() | ||
| return true | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to switch to @pony1k suggested version as it is more compact and we already depend on those libraries
| end | ||
| end | ||
|
|
||
| function config.execute_hooks(action) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function or something very similar feels like to be used/needed in more place in LiMe code, let's try to avoid duplication
Also add an ACTION environment variable containing the same value as passed as a command-line argument.
Some scripts presume they will only be called in the one existing place so they need to be updated also.