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

Logging: change ?LOG macro to ?WF_LOG #142

Merged
merged 1 commit into from
Aug 24, 2021
Merged

Logging: change ?LOG macro to ?WF_LOG #142

merged 1 commit into from
Aug 24, 2021

Conversation

codebykat
Copy link
Contributor

This addresses the issue raised in #136. With props to @bunnylushington as I worked from your proposed solution.

See Slack discussion here: https://erlanger.slack.com/archives/CFV94TLCA/p1629216894001400

I have my doubts about two specifics:

  • I'm undecided about continuing to define LOG when the OTP logger has not been included -- since the parameters differ, it might introduce a more confusing incompatibility than removing it entirely, and encouraging any consumers to move to WF_LOG explicitly.
  • I also somewhat dislike the check for whether the OTP logger is defined, which introduces a requirement that the OTP logger must be included before the Nitrogen library. I'm not sure if there's precedent for forcing a particular order of library inclusion.

I'm wondering if it might be better to avoid the ifndef entirely and force usage of the new logger -- would it make sense to import logger.hrl as a dependency of Nitrogen? (This may be a naive question, as I'm new to Nitrogen and to the Erlang ecosystem.)

n.b. This PR does not update wf:info, wf:error etc to use the new macro.

@choptastic please let me know what you think, and what further work might be needed. Thanks!

@choptastic
Copy link
Member

This is great! Thank you so much for putting in the work!

Forcing the new logger is definitely a good thing, but I have a thing about retaining backwards compatibility as much as possible, so for now, we'll definitely keep the ifndef block and the ?LOG macro around for older OTP versions.

Thanks again!

@choptastic choptastic merged commit 448244c into nitrogen:master Aug 24, 2021
@codebykat codebykat deleted the update/change-log-macro branch August 24, 2021 16:24
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.

2 participants