-
Notifications
You must be signed in to change notification settings - Fork 594
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
Change module autoloading messages to hs.logger debugging statements. #1368
Comments
@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. |
For what it's worth - I think it's a good idea that ALL extensions should use |
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 :-) |
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. |
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 😁). |
Ah, right - I see what you mean. No worries.
Awesome - sounds good to me! |
@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 throughhs.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
?The text was updated successfully, but these errors were encountered: