-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
zapcore: add warning as Level #1429
Conversation
Some packages set their value for WarnLevel to "warning". For easier integration add "warning" as accepted WarnLevel. Example for a package that is using "warning": https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/logrus.go#L68 Signed-off-by: Florian Lehner <[email protected]>
Friendly ping for feedback to @abhinav or @JacobOaks . |
Hey @florianl, I think this change incurs some technical debt for a small amount of flexibility. The current level MarshalText logic is simplified because there is either a capitalized or lower case representation of the each level, where the level is represented in a consistent tense. From my perspective, the special check case in level_test.go makes the level test code a bit more difficult to main. |
Hey @r-hang thanks for the feedback! There are cases where this packages is expected to handle input, that is generated from other packages, that are using the same levels, but slightly different level text - |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1429 +/- ##
=======================================
Coverage 98.69% 98.70%
=======================================
Files 53 53
Lines 2997 3014 +17
=======================================
+ Hits 2958 2975 +17
Misses 31 31
Partials 8 8 ☔ View full report in Codecov by Sentry. |
@r-hang what do you think about having a dedicated test for unmarshaling |
Signed-off-by: Florian Lehner <[email protected]>
@abhinav or @JacobOaks or @r-hang with 126ea63 I have moved the test to guarantee compatability to a dedicated sub test. Please let me know, if this works for you. |
friendly ping to @abhinav or @JacobOaks or @r-hang - do you have some feedback? |
Makes sense to me @florianl! Thanks for helping keep our tests more maintainable and sorry for the delay. |
Signed-off-by: Florian Lehner <[email protected]>
Some packages set their value for WarnLevel to "warning". For easier integration add "warning" as accepted WarnLevel.
Example for a package that is using "warning":
https://github.com/sirupsen/logrus/blob/dd1b4c2e81afc5c255f216a722b012ed26be57df/logrus.go#L68