Skip to content
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

feat(typing): improving @types for AppenderModule #1079

Merged
merged 3 commits into from Jan 18, 2022

Conversation

nicobao
Copy link
Contributor

@nicobao nicobao commented May 13, 2021

Closes #1078

I need help for typing the ConfigParam interface. Do you know what are the relevant types to add? Feel free to edit the PR.

types/log4js.d.ts Outdated Show resolved Hide resolved
@nicobao nicobao marked this pull request as ready for review May 13, 2021
@nicobao
Copy link
Contributor Author

@nicobao nicobao commented May 24, 2021

Hi @nomiddlename , any chance this could be merged?

@nomiddlename
Copy link
Collaborator

@nomiddlename nomiddlename commented May 26, 2021

Hi @nicobao - I've given you permission to merge this PR yourself, let me know if you need any help. As for the correct type to use for ConfigParam, log4js was written way before typescript became popular and retro-fitting types onto it can be a bit tricky. From memory, I think it is an object with a couple of fields like type and name - the part of the full config object that applies to that particular appender. Each appender type can have its own config parameters though.

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jun 20, 2021

Hi @nicobao - I've given you permission to merge this PR yourself, let me know if you need any help. As for the correct type to use for ConfigParam, log4js was written way before typescript became popular and retro-fitting types onto it can be a bit tricky. From memory, I think it is an object with a couple of fields like type and name - the part of the full config object that applies to that particular appender. Each appender type can have its own config parameters though.

Thank you for your reply.
I think the config element is truly the whole Configuration object containing... all the config.
It's not easy to type it, so for now I'll just any and merge it as is.

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jun 20, 2021

Hi @nicobao - I've given you permission to merge this PR yourself, let me know if you need any help. As for the correct type to use for ConfigParam, log4js was written way before typescript became popular and retro-fitting types onto it can be a bit tricky. From memory, I think it is an object with a couple of fields like type and name - the part of the full config object that applies to that particular appender. Each appender type can have its own config parameters though.

However, I don't have permission to merge the PR :(
Maybe that's because the permission timed out?
Anyway, I got " Only those with write access to this repository can merge pull requests. "

@nomiddlename
Copy link
Collaborator

@nomiddlename nomiddlename commented Jun 20, 2021

The invitation to join the log4js org is only valid for 7 days. I've sent you another.

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jun 21, 2021

The invitation to join the log4js org is only valid for 7 days. I've sent you another.

It's weird, I don't see anything on GitHub related to invitation to log4js-node organization. No email, no app notification and nothing in "organization" tab.

@nomiddlename
Copy link
Collaborator

@nomiddlename nomiddlename commented Jun 22, 2021

I think it should have sent you an email, you're definitely in the "pending invitation" list. Screen Shot 2021-06-22 at 10 06 42 am

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jun 22, 2021

My bad, it was in spam & I was checking wrong address's spam 😅
Thank you for your patience!

@lamweili lamweili added this to the 6.4.0 milestone Jan 5, 2022
@lamweili lamweili assigned lamweili and nicobao and unassigned lamweili and nicobao Jan 10, 2022
@lamweili lamweili merged commit c139155 into log4js-node:master Jan 18, 2022
@glasser
Copy link
Contributor

@glasser glasser commented Jan 21, 2022

Are you sure these types are correct? See #1155

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jan 21, 2022

Are you sure these types are correct? See #1155

Hi,
Sorry for the hassle in your codebase.
I will rollback this change swiftly as of now.
I tried to improve the typing of this library, making it more precise, but it seems it wasn't correct.
I will add unit tests to avoid such bugs to get pushed to a new release in the future.

Edit: of course depends on what @peteriman thinks. If @glasser you can help us improving the types it would be even better. I look at it further this evening.

@lamweili
Copy link
Contributor

@lamweili lamweili commented Jan 21, 2022

Or @nicobao would you happen to have the correct fix based on @glasser's report? That'll do as well.
After all, you would know where the appropriate changes are. 😉

@glasser
Copy link
Contributor

@glasser glasser commented Jan 21, 2022

Thanks! I appreciate trying to improve the ability to use TypeScript and I'm sure we can get it to a solid state.

It seems like @nicobao is best suited to make this improvement since @peteriman understands log4js and I understand TypeScript but it looks like you understand both. I'll file a quick PR that just drops the AppenderGenerator type but I really can't promise it's accurate without understanding log4js deeper!

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jan 21, 2022

@glasser I introduced the bug, so it is my responsibility to fix now. I will take care of it. Thank you for reporting the issue! Of course, any help is always appreciated.
My understanding of log4js codebase is actually limited - that was my sole contribution to the library.
I am gonna do my best to find the correct typing and send a PR asap. If I can't find anything conclusive I will just rollback the patch.

@glasser
Copy link
Contributor

@glasser glasser commented Jan 21, 2022

@nicobao I filed a PR #1158 so perhaps review that, and build on it if you'd like? I've invited you to my fork if you'd like to add to my PR.

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jan 21, 2022

Hi, I've rebased your work. I looked at it shortly but didn't have much time this evening.
Will take time for it over the week end.

@nicobao
Copy link
Contributor Author

@nicobao nicobao commented Jan 21, 2022

Because I've rebased be wary of the state of your local repository. Probably better to start by running a git stash --include-untracked followed by a git reset --hard (careful with that last command because it is destructive).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants