Skip to content
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 an ExecuteLastObjectDataManager that attempts to execute after ot… #11

Merged
merged 4 commits into from Apr 17, 2017

Conversation

jzuech3
Copy link

@jzuech3 jzuech3 commented Apr 17, 2017

…her ObjectDataManagers #10 @papachoco @jamadden

@@ -237,6 +253,16 @@ def do(*args, **kwargs):
transaction.get().join(result)
return result

def do_last(*args, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we not use the word "last", here or in the class? That implies promises we can't keep, and it makes no sense if this function/class is used more than once.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendations? I'm stumped :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OrderedNearEndObjectDataManager and do_near_end? (I suppose even NearLast would get the point across.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with either of those. I was hoping the comments would provide the caveat.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only if you read the docstrings. That doesn't help someone just reading the code where this is used.

@coveralls
Copy link

coveralls commented Apr 17, 2017

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling e47ff06 on jzuech3:master into 051b180 on NextThought:master.

execute *after* all other DataManagers have had their say.
See :class:`ObjectDataManager` for the possible arguments.
"""
result = ExecuteLastObjectDataManager(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than reimplementing this, what about something like this:

def do(*args, **kwargs):
    klass = kwargs.pop('datamanager_class', ObjectDataManager) # new argument
    result = klass(*args, **kwargs)
    ... # as before

def do_XXX(*args, **kwargs):
   kwargs['datamanager_class'] = XXXDataManager
   return do(*args, **kwargs)

@cutz
Copy link

cutz commented Apr 17, 2017

out of curiosity how does this work with the single phase commit transactions in the zope transaction manager? IIRC those get sorted to the end by the transaction manager (i've seen this in certain implementations of sqlalchemy). Does this sort before or after those?

@jamadden
Copy link
Member

The zope transaction manager doesn't have a "single phase commit transaction" so I'm not sure what you mean. It only supports two-phase commit. Individual data managers may or may not do something useful during all of those phases.

@cutz
Copy link

cutz commented Apr 17, 2017

Sorry it was the specific sqlalchemy transaction datamanager I was remembering. It tries to sort single phase committers to the end.
https://github.com/zopefoundation/zope.sqlalchemy/blob/master/src/zope/sqlalchemy/datamanager.py#L118

@jamadden
Copy link
Member

~ sorts after zzz so sqlalchemy will still go last. (Which is a good example of why these shouldn't be named Last 😄 )

$ python -c 'print(sorted(["~sqlalchemy:123", "zzz123"]))'
['zzz123', '~sqlalchemy:123']

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling b972eb3 on jzuech3:master into 051b180 on NextThought:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling b972eb3 on jzuech3:master into 051b180 on NextThought:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling b972eb3 on jzuech3:master into 051b180 on NextThought:master.

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor formatting issues.

from ..transactions import TransactionLoop
from ..interfaces import CommitFailedError
from ..interfaces import AbortFailedError
from nti.transactions.interfaces import CommitFailedError
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we preferred absolute imports?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't, they make refactoring harder.

manager_post = do_near_end(call=test_call, args=(10,))
manager3 = do(call=test_call, args=(2,))
transaction.commit()
assert_that(results, contains(0,1,2,10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces between commas, please.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling bb31b75 on jzuech3:master into 051b180 on NextThought:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling bb31b75 on jzuech3:master into 051b180 on NextThought:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling bb31b75 on jzuech3:master into 051b180 on NextThought:master.

Copy link
Member

@jamadden jamadden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a note to CHANGES.rst? And add ..versionadded:: 1.1 lines to the end of the docstrings for the new objects?

Sorry, I should have mentioned that earlier.

@jamadden jamadden merged commit 3c68992 into OpenNTI:master Apr 17, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling 26fcdab on jzuech3:master into 051b180 on NextThought:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 99.149% when pulling 26fcdab on jzuech3:master into 051b180 on NextThought:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants