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
History in context #26
Conversation
routemaster/context.py
Outdated
metadata: Dict[str, Any] = {}, | ||
now: datetime.datetime = None, | ||
feeds: Dict[str, Feed] = {}, | ||
accessed_variables: Iterable[str] = [], |
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'm not sure that having these mutable defaults is a good idea (maybe add flake8-mutable
?).
Perhaps a better way to solve this would be a helper used in tests which handles the non-interesting defaults?
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 fixed these with a test factory method that has the defaults (but not as mutable args).
routemaster/context.py
Outdated
@@ -19,6 +19,7 @@ def __init__( | |||
now: datetime.datetime = None, | |||
feeds: Dict[str, Feed] = {}, | |||
accessed_variables: Iterable[str] = [], | |||
current_history_entry: Optional[Any] = None, |
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.
Do we really not have a type for the entry? Or is it that SqlAlchemy doesn't provide a useful type?
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.
The latter, because we aren't using the ORM. This will change eventually I hope.
I recommend reviewing per-commit here, it should be much simpler.