-
Notifications
You must be signed in to change notification settings - Fork 683
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
[interceptors/validator] feat: add error logging in validator #544
Conversation
used logging.Logger interface to add error logging in validator interceptor addition: grpc-ecosystem#494
made fast fail and logger as optional args addition to that instead of providing values dynamically at the time of initialization made it more dynamic
@bwplotka have updated the implementation do let me your feedback. if it looks good🤞🤞 then I will update the doc and test cases too. just wanted to be sure about the implementation first 🙈. I have taken inspiration from logging module |
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.
Amazing work! Generally great direction, just some nits.
updated args based on review
restructured if statement in-order to make code execution based on shouldFailFast flag more relevant.
restructured code in order to separate the concern. ie: in terms of code struct and testcases wise.
modified testcases based on the current modifications made in the code base.
@bwplotka can you help me to figure out what's the lint issue? |
Just run "make lint" locally and commit changes (some automatic fixes got applied) |
updated code based on reviews
make lint again please until it does not show any more errors locally (: |
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.
LGTM! Just lint. Thanks!
I can also just merge and fix lint on my own, that's fine - I plan to do one more thing.
sure go ahead actually, I tried running lint at my end but it's missing the initialization of |
What is an error? Because GOIMPORTS is defined in Makefile variable file in .bingo/Variables.mk Anyway, merging, thanks! |
used logging.Logger interface to
add error logging in validator
interceptor
addition: #494