-
Notifications
You must be signed in to change notification settings - Fork 16
Added setters for log enablement, all-log enablement, and log level. #4
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
Conversation
To make this make sense, also had to add code that checks whether logs are enabled at call time rather than checking the value of `IsEnabled` which is set at instantiation time. The value of IsEnabled is updated whenever an internal method that uses it checks for enablement. Added a little bit of documentation to existing and new functions.
|
Forgot to reference #3 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea 👍
|
thanks a lot, I'm leaving some comments |
settings.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove this, and change enabled allEnabled to capital case
|
By all means; all of the above oddities derived from me avoiding changing the existing API. If you feel like changing APIs there are much cleaner ways to expose the required functionality without kludges. For e.g.: If I were breaking the API, I'd just drop the Likewise the |
settings.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be nicer to take advantage of constants;
const (
TIMER_LEVEL = 1
ERROR_LEVEL = 2
INFO_LEVEL = 3
)
|
@cathalgarvey I think we can change the API little bit. This PR will enable logger to be used programmatically for first time, so it's ok change things for good looking API. Thanks a lot! |
|
Looking over Logger's importers on Godoc: https://godoc.org/github.com/azer/logger?importers ..Looks like nobody is using the sections of the API that I'm changing here. Well, maybe you are but I'm not looking at your other projects, because it's your API to break. :) So we're safe. |
…viour of 'porcelain' API.
|
I have not yet tested this, I'm just pushing while in-progress. Big changes:
Other:
Next steps: |
|
Derp, typo posted early. Next steps I'd recommend:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree with you 👍 anonymous struct would be much better.
Added setters for log enablement, all-log enablement, and log level.
|
sorry for late merge, thanks for the PR 👍 |
|
@cathalgarvey I can add you as collaborator if you'd like to merge your PRs quicker |
To make this make sense, also had to add code that checks whether
logs are enabled at call time rather than checking the value of
IsEnabledwhich is set at instantiation time. The value of IsEnabledis updated whenever an internal method that uses it checks for
enablement.
Added a little bit of documentation to existing and new functions.