Logging needs attention about nvim-ide

[ldelossa]

Here is an example in "workspace.lua"

nvim-ide/lua/ide/workspaces/workspace.lua

Line 44 in ca17635

self.config = vim.deepcopy(ide.config)

As you can see, you should be able to access config like a global with "require("ide").config". Im not sure why this wouldnt work for you.

[BeeverFeever]

I will give it a quick try but you might have to do it because I'm going away and won't be able to work on it.

[BeeverFeever]

How can I get the log_level value from the config into the logger.lua file?
I tried to do require('ide') at the top of the logger file and then access the config table from that but it gave an error saying there was an error loading the module.

[BeeverFeever]

Hmm ok, i'll see if i might have been doing something wrong. thanks

[BeeverFeever]

After playing around with things I still can't get the require to work. The error it gives me is loop or previous error loading module 'ide' and I can't find anywhere where it would be a loop (unless I don't completely understand lua's require system).

[distek]

Hey @ldelossa I was looking in to this and I see what BeeverFeever was saying; There's a module dep loop when it comes to requiring 'ide' inside of logger due to the panels already requiring it.

I believe we talked about it before, but how would you feel about if, instead of obtaining a copy of the config in whichever modules need it, we global the config and copy in to it on init so we can just call IDEConfig.whatever instead?

Not sure what I was thinking there, that's not how globals work. Could we instead just cascade down the config? Otherwise, I'm not sure how else we can get the config into logger properly.

[ldelossa]

If we move config out of init.lua, to its own file, and update all the imports, this solves the issue, right?

The config module can be "export only" which should break any loops.

[distek]

I think that will work. Do you want a PR for it?

[ldelossa]

Yup!

[distek]

@BeeverFeever Do you want to work on this? If not, I can go at it later today.

[distek]

Sounds good, just let me know.

[BeeverFeever]

Ok will do.

[BeeverFeever]

Quick question, with the panel_groups in the config, do you think they should just be set in the setup function in init.lua before mergeing the users config? Otherwise a require is needed in config.lua(the new file with the default config)

[distek]

I think doing the requires in the config.lua is fine? Maybe by doing something like:

local bufferlist      = require('ide.components.bufferlist').Name

[BeeverFeever]

Ok I'll see if that might work.

[ldelossa]

Should work, the init.lua files in each component dir dont need to refer to config.

[BeeverFeever]

It still doesn't work. The only way I can think of getting it to work is by hard coding the values in. So for example you just put the name of the components you want as a string rather than using a require.

[BeeverFeever]

@ldelossa The problem is that the components are referring to logger though.

[ldelossa]

@BeeverFeever yes I see.

main...refactor-for-logger - Here is a base refactor, I think this is fine, we'll just expose a method in the logger which sets the log level, and not give it the full config.

Want to use this as base to work the rest of the PR out?

[BeeverFeever]

Yeah sure, that looks pretty good.

[BeeverFeever]

@ldelossa Just wondering, do you want to use the vim.log.levels. for the logger levels or could I go with something where we set a table of log levels, like:
(in logger.lua)

local log_levels = {
    ['debug'] = 1,
    ['error'] = 2,
    ['warning'] = 3,
    ['info'] = 4,
}

and then use those values to detect whether to log the message or not?

[BeeverFeever]

This is what I've got so far. BeeverFeever/nvim-ide@47225a6

[ldelossa]

The 'set_log_level' probably take either a string, or an integer. But the log_level variable should probably be an int, so you can determine if you should log with a '>' or '<' evaluation.

[BeeverFeever]

Ok sounds good. I will make those changes and tidy it up then make a pr if it looks good to you otherwise?

[BeeverFeever]

Also one more thing sorry, how would I run the logger tests?

[ldelossa]

Your diff looks fine, maybe we add a function which checks the incoming log_level variable to see if its a string or int, and attempt to return an integer.

Then we can do one more pass which removes all the strings in favor of using vim.log.levels directly. If we feel thats better.

[BeeverFeever]

Yeah that sounds better than using strings. Just wanted to make sure, did you want it to only log the level that is set by Logger.log_level or did you want it to log the set level and anything 'higher' than it?

[ldelossa]

Log level and higher. So if you set for "debug" it should log debug and all higher levels.

Tests are best effort so far. You can add them if youd like.

[BeeverFeever]

Ok thanks, how would I run the tests?

[ldelossa]

We don't have automated tests at this point. Most of the time I run tests manually to confirm something works.

[BeeverFeever]

Ahh ok fair enough

[BeeverFeever]

I have done a bit but I ran into a problem where it seemed like Logger.log_level was just being set to nil and I was getting an error saying attempt to compare number with nil. I have no clue why this is happening.

BeeverFeever/nvim-ide@020a211
I hate leaving this broken right now but I just don't have any more time right now to work on it. If someone else wants to look at it they can or I will pick it up again in a couple days to see if I can get it to work.

[ldelossa]

@distek did you want to take it from here? If not I will.

@BeeverFeever no worries - glad you got to take a stab at it.

[ldelossa]

Still needs an over-all logging pass, to place info logs in more appropriate places, but the leveled logging stuff is done.