search

Hammerspoon / hammerspoon

Staggeringly powerful macOS desktop automation with Lua
http://www.hammerspoon.org
MIT License
11.86k stars 578 forks source link

Change module autoloading messages to hs.logger debugging statements. #1368

Open asmagill opened 7 years ago

asmagill commented 7 years ago

@cmsj, I've noticed that most of your spoons don't use require and rely on module autoloading... if this is going to become the convention, then I think we should revisit the question of whether or not to add a flag to suppress these particular console messages (or at least issue them through hs.logger... that's probably better because then I can still get at them when I'm on a debugging tear).

I really don't like messages in the console that I haven't chosen to be there... to me it indicates something isn't right and needs to be fixed, or at least I use it that way...

Thoughts? Objections if I add a flag that can be set with hs.settings?

cmsj commented 7 years ago

@asmagill I'd be fine with having them be logger messages at a lower-than-default level.

I've never really liked the setting to disable auto loading and I suspect it's almost never used (and I'd support removing it). I think it makes sense for our extensions to be explicit about require()ing, but I don't think Spoons matter - most people aren't going to be explicit there, anymore than they are in their init.lua.

latenitefilms commented 7 years ago

For what it's worth - I think it's a good idea that ALL extensions should use hs.logger to submit content to the Console (as opposed to print). I also agree that the auto-loading messages should probably be at a debug level. I can't think of a good reason why you'd want to disable module auto-loading, so I also have no objections if you were to remove this toggle.

asmagill commented 7 years ago

the thinking behind the disabling of module auto-loading was because the primary developers of Hammerspoon's predecessor, Mjolnir, wanted a strong focus on minimalism... minimal memory footprint, minimal built-in modules, etc. The fork occurred in part because some felt that at least some of the modules being developed were too useful to be optional. I personally fell somewhere in-between but do agree that disk space is cheap, so as long as you watch what you're loading, having the modules sitting in the app bundle unused doesn't bother me.

Of course by this point, I probably have them all loaded anyways, given how much my configuration has grown.

I originally supported the option to disable the autoloading, but after about a month turned it back on and haven't really messed with it since. I'm not against removing it at this time, but I also don't think time should be spent on it... as long as its not in the way of anything, my thought is why bother?

I think the uielement message is the only remaining hold-out in the core modules for "proper" use of require... it just hasn't bothered me enough to track down, but I think we probably should at some point. Complexity isn't a good excuse for what is more properly termed "a bloody mess" but the last time I worked on hs.uielement, I broke it (and we released it, to my chagrin), so... yeah... while I want it fixed, I too am reluctant until I have time to really dedicate to it :-)

cmsj commented 7 years ago

I haven't checked, but I think that the way hs.application, window and uielement all depend on each other, means they probably can't all neatly use require().

@latenitefilms yes, everything should be using hs.logger, but it's much newer than many of our extensions, so things don't use it yet, also I'm not in the habit of using it.

Also also, I still want to move the logging core to LuaSkin and have hs.logger just be a shim for talking to that.

asmagill commented 7 years ago

The fact that it works at all with auto-loading means that there is at least one sequence of requires that works... but the effort to figure out what it might be is not something I want to worry about anytime soon, other then maybe for completeness sake (and, yeah, the chance to pump my fist in the air and do a little dance 😁).

latenitefilms commented 7 years ago

I haven't checked, but I think that the way hs.application, window and uielement all depend on each other, means they probably can't all neatly use require().

Ah, right - I see what you mean. No worries.

Also also, I still want to move the logging core to LuaSkin and have hs.logger just be a shim for talking to that.

Awesome - sounds good to me!