-
Notifications
You must be signed in to change notification settings - Fork 3
Deprecate clock_tick context manager #39
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
base: master
Are you sure you want to change the base?
Conversation
|
Resolves #28 |
temporal_sqlalchemy/bases.py
Outdated
|
|
||
| def __get__(self, instance, owner): | ||
| if not instance: | ||
| return 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.
You might want to use return self here, because otherwise any attempt to access reset_activity or activity_required from an instance of a class using it will crash with an AttributeError:
A minimal example:
import types
ns = types.SimpleNamespace(activity=1)
# This works
ActivityState.reset_activity(ns, None)
class B:
astate = ActivityState()
# This crashes
B.astate.reset_activity(ns, None)
temporal_sqlalchemy/bases.py
Outdated
|
|
||
| def __get__(self, instance, owner): | ||
| if not instance: | ||
| return 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.
Same idea here about returning self.
| clock_properties['activity'] = \ | ||
| orm.relationship(lambda: activity_class, backref=backref_name) | ||
| cls.activity = ActivityState() | ||
| event.listen(cls, 'expire', ActivityState.reset_activity) |
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.
@NicoleZuckerman this the expire for the activity descriptor
| ) | ||
|
|
||
| cls.current_clock = ClockState() | ||
| event.listen(cls, 'expire', ClockState.reset_tick) |
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.
@NicoleZuckerman this is the expire for the current_clock descriptor
|
relevant sqlalchemy documentation on instance events: http://docs.sqlalchemy.org/en/rel_1_1/orm/events.html#instance-events |
temporal_sqlalchemy/bases.py
Outdated
| if not instance: | ||
| return self | ||
|
|
||
| vclock = getattr(instance, 'vclock') or 0 |
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.
Using getattr with a string constant like this is equivalent to instance.vclock. This will still crash with an AttributeError unless you pass a default value as the third argument. Did you mean to do that?
vclock = getattr(instance, 'vclock', 0)| setattr(instance, '__temporal_current_tick', clock_tick) | ||
| instance.clock.append(clock_tick) | ||
|
|
||
| return getattr(instance, '__temporal_current_tick') |
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.
Same note here about using getattr without a default argument.
However, be warned: attributes starting with double underscores may not behave as you expect. Python has a concept of "private variables" that may result in an AttributeError getting thrown.
From the docs:
Any identifier of the form
__spam(at least two leading underscores, at most one trailing underscore) is textually replaced with_classname__spam, where classname is the current class name with leading underscore(s) stripped. This mangling is done without regard to the syntactic position of the identifier, as long as it occurs within the definition of a class.
Because it's required to occur inside the class definition I think this will be okay, but I thought I'd point it out.
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.
yeah, I think in this case it's okay, but it's a good call out
temporal_sqlalchemy/bases.py
Outdated
| DeprecationWarning) | ||
| if self.temporal_options.activity_cls: | ||
| if not activity: | ||
| raise ValueError |
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.
Can we put an error message here please? It'll make debugging easier. I think the original error message might suffice.
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.
👍 should probably be:
raise ValueError("activity is missing on edit") from None
temporal_sqlalchemy/bases.py
Outdated
| yield self | ||
|
|
||
| session.add(new_clock_tick) | ||
| return |
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.
return isn't necessary at the end of a function. You can remove 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.
I left it in so it looks less confusing to me, but I'm strange that way 😆
multigl
left a comment
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.
looking good, looking real good
|
|
||
| # clear out db | ||
| transaction.rollback() | ||
| second_session.close() |
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 pretty neat, might also want to refetch the model from the first session and see that the vclock is two there
tests/test_temporal_with_activity.py
Outdated
| activity=create_activity) | ||
| session.add(t) | ||
|
|
||
| assert getattr(t, '__temporal_current_tick', 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.
I wonder if we want to assert that it's an instance of ClockTick (or whatever it actually is) and has properties we expect?
| assert t.activity | ||
| session.commit() | ||
| assert not getattr(t, '__temporal_current_tick', None) | ||
| assert not t.activity |
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.
yesssss
temporal_sqlalchemy/bases.py
Outdated
|
|
||
| class ActivityState: | ||
| def __set__(self, instance, value): | ||
| assert instance.temporal_options.activity_cls, "Make this better Joey" |
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.
Can we change this error message to something a bit more helpful? 🙂 Perhaps:
if instance.temporal_options.activity_cls:
raise RuntimeError(
"Can't set activity state on instance of %r because the activity class is None."
% type(instance).__name__)(That's assuming that my interpretation of this assertion is correct.)
clock_tickcontext manageractivity_cls