Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: support AuditLog and RequestLog protos #274
feat: support AuditLog and RequestLog protos #274
Changes from 15 commits
5c8bd7e
b15b20e
f389c33
7a75fca
646f717
06cf4f5
549566e
fc36053
568bfcf
05f3283
83e3111
5ce683f
dc90e72
66d9c62
afddbf9
25c39df
2aa35ca
2d89fa1
2c8e136
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
this is a side note for logging language syntax and not to this specific line:
I find it ugly when we concatenate strings to form a query. Perhaps, we should think about creation of an abstraction that will make forming these filters more intuitive when you read the code.
For example:
Instead of
it could be
This could be a separate task and not part of this PR
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.
That's an interesting idea. I'm not sure if that specific example would be seen as idiomatic for Python, but there's probably something interesting we could do with this
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.
2 cents: I recall in Python (and Node), this kind of interpolation is preferred
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.
please, no abbreviations in test or other method names
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.
"to_api_repr" is an existing function that we can't change here without breaking the API. We could add this to the list for v3.0.0 changes (but it may not be worth the user confusion)