Add lualogging initial support #38
Conversation
types/lualogging/logging.d.tl
Outdated
| local record Types | ||
| Level: Level | ||
| Append: Append | ||
| Log: Log | ||
| ConsoleDestination: ConsoleDestination | ||
| ConsoleParameters: ConsoleParameters | ||
| FileParameters: FileParameters | ||
| RollingFileParameters: RollingFileParameters | ||
| end |
There was a problem hiding this comment.
This record is only here to create a structure that allow to expose the types defined in this file.
types/lualogging/logging.d.tl
Outdated
| rolling_file: Append<RollingFileParameters> | ||
| -- TODO : add more appenders | ||
|
|
||
| __types: Types |
There was a problem hiding this comment.
I expose the types with this additional property. I think it's ok to add this property (that doesn't exist in the library) because it's "teal only", and doesn't add anything that could be translated to Lua.
In the usage, the user can now refer to types from this file somewhere else in their code: local myParams: logging.__types.ConsoleParameters = { foo }. The usage of __types will not be translated to anything in Lua, so there is no impact for adding this property.
Note that I named it __types with this double _ to make it feels like a meta property. I have no strong opinion about it.
There was a problem hiding this comment.
I think you could just put those in the module's main record. In other definition files from this repo, we just went ahead and exposed PascalCase type names as part of the public API (see for example luafilesystem), assuming those won't clash with the usual lowercase or snake_case entries exposed by the Lua APIs we're typing. For the Teal user, using logging.ConsoleParameters rather than logging.__types.ConsoleParameters would be a bit more comfortable, I think, and I believe clashes with future lualogging APIs would be extremely unlikely.
There was a problem hiding this comment.
Thank you for pointing out the luafilesystem example. I have checked some other types definition to see how similar needs were solved. I somehow missed it...
I actually like the idea to include the inner-record directly in the record. It gives some scope to it. The PascalCase should solve the clashes with properties defined by the module API (for a lot of Lua module), and help to identify the property as a type name (this “convention” is also used by typescript). So I'm all in!
| FATAL: Level | ||
| OFF: Level | ||
|
|
||
| new: function(append: function(self: Log, level: Level, msg: string | function), startLevel: Level): Log |
There was a problem hiding this comment.
Here is the real reason why I had to expose types:
The new function wants a callback parameter with specific parameters. Either, the user can't type the parameters (because they can't reference the Log and Level types), or we would have to type them here with something more open (any?) which is the opposite of the idea behind using a typed language.
local log = logging.new(function(_, level, msg) print(level, msg) end, logging.DEBUG) -- argument 1: argument 1: got <unknown type>, expected Log
local log2 = logging.new(function(_: logging.__types.Log, level: logging.__types.Level, msg: string) print(level, msg) end, logging.DEBUG) -- Now it works
types/lualogging/logging.d.tl
Outdated
| -- undocumented/private APIs | ||
| compilePattern: function(pattern: any): function(): string | ||
| prepareLogMsg: function(lpattern: string, dpattern: string, level: Level, message: string): string | ||
| tostring: function(value: any): string |
There was a problem hiding this comment.
I'm not sure whether it's welcomed to add this kind of private members to the type definitions (?)
There was a problem hiding this comment.
I don't think it's necessary — do you need to use them? If not I think I'd rather keep them out.
There was a problem hiding this comment.
I guess the user is not supposed to use these functions since they are not documented and looks like private API to me. I'm removing them.
| -- Deprecated | ||
| getDeprecatedParams: function(lst: table, ...: any) | ||
|
|
||
| -- Appenders (dynamically added to logging when required) |
There was a problem hiding this comment.
This is something I asked in the chat.
Teal can't dynamically add fields to record, so we have to add all the appenders here, even if they actually only exist after requireing the corresponding module. (Example of how console is added https://github.com/lunarmodules/lualogging/blob/e900e4a425eac14d551a576f63969291b89548a0/src/logging/console.lua#L27)
types/lualogging/logging.d.tl
Outdated
| console: Append<ConsoleParameters> | ||
| file: Append<FileParameters> | ||
| rolling_file: Append<RollingFileParameters> | ||
| -- TODO : add more appenders |
There was a problem hiding this comment.
Here is the uncompleted part I mentioned on the message. I think it's ok to ship without all the appenders. They can be added later when someone needs them.
There was a problem hiding this comment.
I agree, and incomplete definition file is better than a missing one!
Use the map syntax with spaces on both sides of the `:` character.
|
Thank you, @hishamhm, for the review comments! I have updated the type definition according to your feedbacks. It's now ready for a new pass 😄 |
|
@Aire-One Thank you for addressing the reviews, I think this is good to go! Merged! |
Hello,
This adds type definition for the lualoging library.
It's not complete because I haven't set up and tried every "appender" types. So I'm not including them in the current type definitions (I currently haven't planed to add them).
I have mainly tested the definition by running the code examples from the documentation.
Note that some aspect of the type definitions I wrote can be subject to discussion (see my comments).
Any feedbacks are welcomed 😄