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

[Merged by Bors] - Remove global initialization of logger #174

Closed

Conversation

marcmo
Copy link
Contributor

@marcmo marcmo commented Aug 19, 2021

  • fixes Allow to configure a custom logging backend #134
  • Provide macro to run code on initialization without introducing
    additional direct dependencies to client projects.
  • without this global initialization applications need to setup their
    own logging infrastructure at startup
  • Adapted function example with the use of a custom logger

@nicholastmosher
Copy link
Contributor

Hi @marcmo, thanks for the PR! We're hoping to test this out and give some feedback soon 👍

@marcmo
Copy link
Contributor Author

marcmo commented Sep 28, 2021

@nicholastmosher is this still something you consider? Then I would add some documentation for it.

Copy link
Collaborator

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for lates response. I think there is forward step to resolving this issue

examples/function/Cargo.toml Outdated Show resolved Hide resolved
nj-core/src/lib.rs Show resolved Hide resolved
- fixes infinyon#134
- Provide macro to run code on initialization without introducing
additional direct dependencies to client projects.
- without this global initialization applications need to setup their
own logging infrastructure at startup
- Adapted logger example with the use of a custom logger
@marcmo marcmo force-pushed the remove_fix_logger_initialization branch from ec10628 to 03643a8 Compare September 28, 2021 17:59
@marcmo
Copy link
Contributor Author

marcmo commented Sep 28, 2021

@sehz I created a separate example to demonstrate the custom logger.
By regression you mean that I removed a public function probably...would it be sufficient to add the init_logger() function back in but do not call it?

@sehz
Copy link
Collaborator

sehz commented Sep 28, 2021

@sehz I created a separate example to demonstrate the custom logger.
By regression you mean that I removed a public function probably...would it be sufficient to add the init_logger() function back in but do not call it?

By default (if custom init is not called), we need to initialize logger. Otherwise, everyone need to initialize logger explicitly. I can take look see if there is way to insert global logger if not initialized

@marcmo
Copy link
Contributor Author

marcmo commented Sep 30, 2021

IMHO initialization of a logging framework is something every application should do individually. At least, that's how almost all libraries/frameworks I know do it.
But maybe I'm thinking of node-bindgen too much of a separate project while it is meant to be used in the fluvio world...

@sehz
Copy link
Collaborator

sehz commented Sep 30, 2021

Hmm. I think you have valid point. This is meant to to be used in any library/application. Default initializing maybe too opinionated and also may be more complexity than warranted. In that case, this PR seems to be good stage

Copy link
Collaborator

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pushing this thru

Copy link
Collaborator

@sehz sehz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will bump up version change after this goes thru

@sehz
Copy link
Collaborator

sehz commented Sep 30, 2021

bors r+

bors bot pushed a commit that referenced this pull request Sep 30, 2021
- fixes #134
- Provide macro to run code on initialization without introducing
additional direct dependencies to client projects.
- without this global initialization applications need to setup their
own logging infrastructure at startup
- Adapted function example with the use of a custom logger
@bors
Copy link

bors bot commented Sep 30, 2021

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title Remove global initialization of logger [Merged by Bors] - Remove global initialization of logger Sep 30, 2021
@bors bors bot closed this Sep 30, 2021
@marcmo marcmo deleted the remove_fix_logger_initialization branch October 1, 2021 08:26
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.

Allow to configure a custom logging backend
3 participants