-
Notifications
You must be signed in to change notification settings - Fork 19
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
Add logging support #47
Conversation
lib/mllp/logger.ex
Outdated
|
||
defp parse_data([]), do: "" | ||
|
||
defp parse_data(data) do |
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.
Hey @pggalaviz, what's the advantage of this vs inspect/1
?
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.
Mostly it provides a cosmetic formatted log message for N number of data keys passed in the Keyword.
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 see. So, a few issues and stepping back :
-
inspect/1 basically does most of this for us. i.e.,
inspect([key1: :value1, key2: :value2])
will emit[key1: :value1, key2: :value2]
instead ofkey1: :value1, ...
. So generally just a difference in the inclusion of brackets and so forth. So I'm not sure there's a big gain there. -
inspect/1
is already optimized, we'd want to benchmark this against inspect I think. -
It looks like we're confining ourselves to a keyword list, which may be fine, but we may want to end up inspecting other types of data.
I think if we step back the main benefit we're getting here is [MLLP]
prefix, which is good as consistency is always a win, but I'm not sure the module is a big benefit overall.
I think the issue at hand as far as logging support goes is that simply more meaningful logging. For example, in MLLP.Client there's not a lot, especially in regards to debug logs.
I think we failed to add context to the issue your working on in that regard.
Thoughts?
I think if we step back we can see the main benefit we're getting is
I think when we stand back the main benefit we get out of this.
@vikas15bhardwaj Might have some thoughts on this as well since he had to do a lot of MLLP related work recently.
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 agree, as I said the main benefit is consistency and some cosmetics, the issue with inspect/1
is that it will add "\
around any given string. But considering performance I guess inspect is the way to go.
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.
Changed implementation to use inspect/1
on 6d84786
Closes for now |
Addresses issue #26