-
Notifications
You must be signed in to change notification settings - Fork 17
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 _measure_time time function #32
Add _measure_time time function #32
Conversation
src/pymongo_migrate/mongo_migrate.py
Outdated
@@ -32,6 +32,13 @@ def _deserialize(data, cls): | |||
return cls(**data) | |||
|
|||
|
|||
def _measure_time(func, argument): |
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.
A function that get the function to execute and an argument, call the function with the given argument and return the time of executions.
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.
make it a contextmanager - it will be much easier to use
two alternative implementations:
from typing import Optional
class MeasureTime:
def __init__(self):
self.start = None
self.stop = None
@staticmethod
def time() -> float:
return time.time()
@property
def elapsed(self) -> Optional[float]:
if self.start is None:
return None
return (self.stop or self.time()) - self.start
def __enter__(self):
self.start = self.time()
return self
def __exit__(self, exc_type, exc_val, exc_tb):
self.stop = self.time()
mt = MeasureTime()
with mt:
func(args)
print(f"func call lasted {mt.elapsed}s")
import contextlib
@contextlib.contextmanager
def measure_time():
start = time.time()
def elapsed():
return time.time() - start
yield elapsed
with measure_time() as elapsed:
func(args)
print(f"func call lasted {elapsed()}s")
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.
Thanks,
That great, I chose the second implementation you proposed, it nicer.
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.
After some struggling with the second approach, moved to the first one (define a class).
src/pymongo_migrate/mongo_migrate.py
Outdated
exec_time = _measure_time(migration.upgrade, self.db) | ||
self.logger.info("Execution time of %r: %s", migration.name, exec_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.
Log the execution time.
src/pymongo_migrate/mongo_migrate.py
Outdated
exec_time = _measure_time(migration.downgrade, self.db) | ||
self.logger.info("Execution time of %r: %s", migration.name, exec_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.
Log the execution time.
Fix missing parentheses
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 make sure make check
passes on your machine
from what I have seen the automated formatter of the code was not run
src/pymongo_migrate/mongo_migrate.py
Outdated
@@ -32,6 +32,13 @@ def _deserialize(data, cls): | |||
return cls(**data) | |||
|
|||
|
|||
def _measure_time(func, argument): |
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.
make it a contextmanager - it will be much easier to use
two alternative implementations:
from typing import Optional
class MeasureTime:
def __init__(self):
self.start = None
self.stop = None
@staticmethod
def time() -> float:
return time.time()
@property
def elapsed(self) -> Optional[float]:
if self.start is None:
return None
return (self.stop or self.time()) - self.start
def __enter__(self):
self.start = self.time()
return self
def __exit__(self, exc_type, exc_val, exc_tb):
self.stop = self.time()
mt = MeasureTime()
with mt:
func(args)
print(f"func call lasted {mt.elapsed}s")
import contextlib
@contextlib.contextmanager
def measure_time():
start = time.time()
def elapsed():
return time.time() - start
yield elapsed
with measure_time() as elapsed:
func(args)
print(f"func call lasted {elapsed()}s")
Change the `measure_time` function to work as a contextmanager.
src/pymongo_migrate/mongo_migrate.py
Outdated
migration.upgrade(self.db) | ||
with measure_time as elapsed: | ||
migration.upgrade(self.db) | ||
self.logger.info(f"Execution time of {migration.name}: {elapsed()}s") |
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 used here an f-string
to pass content to the logger, although the rest of the file usage %r
style. Is it ok?
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.
know I think about it - no, please use %-format style
my reason for sticking with %-format style for logging is the way tools like sentry handle error log aggregation - as it integrates with logging on Python level it can easily aggregate stuff using the same format string. If you use f"" formatting each message looks like a different one to it and it cannot aggregate.
So yea, %-format is not as nice, but preventing duplicates in Sentry is much bigger concern to me.
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.
Fixed.
fix the error: `Imports are incorrectly sorted and/or formatted.`
love it btw https://hacktoberfest.digitalocean.com/ is on and you are off to a good start ;) |
Thanks, I just joined the challenge 🥇 |
No description provided.