-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(configureable-logger): add support of configurable logger #164
Conversation
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.
Found quite a few issues. I've left feedback. Please add tests.
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've requested code changes. Please see my comments.
Besides this, these things need to be fixed before the next review:
- PR must pass checks, including tests and quality checks.
- PR must not contain formatting changes. Unrelated formatting changes or refactoring can be done in a separate PR other than this feature PR.
packages/authentication-adapters/src/compositeAuthenticationAdapter.ts
Outdated
Show resolved
Hide resolved
… maskSensitiveHeaders headers
…om/apimatic/apimatic-js-runtime into configurable-logger-implementation
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.
See my comments for the requested changes.
It seems like some of my previous feedback from the last review was not acted upon. Please ensure that all issues reported in this review are taken care of.
…s used in APILogger
* mergeLoggingOptions provides a way for SDKs to safely override options while maintaining foward compatibility with future Core library releases. * PartialLoggingOptions interfaces can be used in SDK configuration interface.
…ed vulnerability using regex
….ts tests to unit tests
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've left my feedback in multiple places.
Also, please create a separate issue for fixing the quality issues in ApiLogger if you do not intend to fix them in this PR and link it here. I see that you have it added to the .codeclimate.yaml ignore list.
|
…t accross languages
…nother access-control-allow-origin in headersToWhitelist
…use them in error-handling interceptor
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 work on finding the solution.
There are two minor changes requested.
|
Closes #163