-
-
Notifications
You must be signed in to change notification settings - Fork 278
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
274 make beautiful optional #275
Conversation
5b8752f
to
88e37c2
Compare
Very nice as "nice" ;-) Just one last idea, which has to be tested (it is idea only).
and use the local variable for default configuration section. |
fs-widget/fs-widget.lua
Outdated
local mounts = args.mounts or {'/'} | ||
local timeout = args.timeout or 60 | ||
|
||
if beautiful ~= nil then |
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.
Is this working? You require the beautiful library, so the beautiful variable is the instance. It has to exist.
Maybe you should replace this check by :
if not #beautiful.get() then
Or another more robust check would be to test each field you need from beautiful.
(Also, I personally don't think this is a good implementation. You basically replace the module level config
values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)
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 is nice example when the unit tests can detect fast these errors. I know that this is lua, not java, or C#, but unit tests are necessary for every language. It is larger investment from the beginning, but this investment will return very fast.
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.
Or another more robust check would be to test each field you need from beautiful.
Like this?
config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal
(Also, I personally don't think this is a good implementation. You basically replace the module level
config
values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)
You're right, maybe it would be better to shadow config
in the constructor then? Something like this:
local function worker(user_args)
local args = user_args or {}
loca config = config
if #beautiful.get() ~= nil then
config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal
What do you think?
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.
Or another more robust check would be to test each field you need from beautiful.
Like this?
config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normal
It can even be easier with:
config.widget_bar_color = config.widget_bar_color or beautiful.fg_normal
The Lua or
operator returns the first "true" value. SO the nullity check is handled by or
.
(Also, I personally don't think this is a good implementation. You basically replace the module level
config
values from the instance constructor. I mean, it works but it feels like encapsulation breakage since an instance is altering a class level property that can affect other instances.)You're right, maybe it would be better to shadow
config
in the constructor then? Something like this:local function worker(user_args) local args = user_args or {} loca config = config if #beautiful.get() ~= nil then config.widget_bar_color = beautiful.fg_normal == nil and config.widget_bar_color or beautiful.fg_normalWhat do you think?
I'm not really a fan of shadowing values. I don't even think it would work as expected here, beautiful.fg_normal == nil and config.widget_bar_color
I think it will use the "new" config table instead of the global.
I did a quick PR to show you what I have in mind with this pattern.
[fs-widget] Implement an alternative config pattern
Do not rely on beautiful, as it might be initialized after the widget, or not initialized at all. Instead, use default valuesm which can be overriden by user.