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
♻️ [#2060] Replace get_paginated_results with pagination_helper #995
♻️ [#2060] Replace get_paginated_results with pagination_helper #995
Changes from 1 commit
860660e
91e6bd5
1dce7e0
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.
yield from (moment for moment in moments if...)
retrieve_objectcontactmoment
). Is this to allow for possible extensions where we actually use multipleobjectcontactmomenten
from this? Otherwise this and the following function could be collapsed.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 API so best not to mix lists and generator return values (because you don't know how it is used).
(also: the example would be
return (moment for moment in moments if...)
(adding yield from just adds another generator around a generator))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 could collapse it into one function
retrieve_objectcontactmoment
, sinceretrieve_objectcontactmomenten_for_object_type
is indeed only used by that function currently. Though we might need this in the future, so that's why I added it initiallyThere 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.
logger.warning("no service defined for {type}", type=type_)
(better to avoid f-string interpolation with logging)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.
@pi-sigma Nooooo. Where does this information/advice come from?
.format()
is much more dangerous because it'll actually raise runtime errors if you miss a replacement placeholder, while f-strings would catch it at parse time.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.
logger.warning("no service defined for %s", type_)
is actually the correct form (see https://docs.python.org/3/howto/logging.html#optimization)(
"no service defined for {type}", type=type_
is afaik not supported by the 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.
That links to the optimization section. Here is the bit about variables: https://docs.python.org/3/howto/logging.html#logging-variable-data, which doesn't mention a correct form but says something about support.
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 thought
logger.warning("no service defined for {type}", type=type_)
was equivalent tologger.warning("no service defined for %s", type_)
; perhaps that's not the case.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.
log methods only allow pos. args (except for
exc_info
and similar). I also think that apart from performance (which can be meaningless is most cases), it allows easier grouping of log messages in tools like SentryThere 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.
@Bartvaderkin This not not (merely) about performance. The main motivation is to facilitate log aggregation in Sentry. If you do
logger.warning("no service defined for %s", type_)
, the logger can group together logs with different instances under the same label. Both f-strings and.format
should be avoided. Like I said, I didn't think oflogger.warning("no service defined for {type}", type=type_)
as equivalent to.format()
logging.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.
Are we doing log aggregation in tools like Sentry in this project? I don't think the existing logs are setup for that so that would be introducing something new.
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.
But it's true that with deferred string interpolation, it would only error at runtime if there's a mismatch in the number of arguments. I think linting rules exist to warn on this issue though (logging-too-many-args from Pylint)
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.
We do have Sentry set up for test/acceptance/prod and if
logger.warning(f"no service defined for {type_}")
gets triggered for two differenttype_
s, it will create two separate issues in Sentry, whereaslogger.warning(f"no service defined for %s", type_)
will group them together