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

new: StandardDatabase.fetch_transaction (#329) #331

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

Moortiii
Copy link
Contributor

Resolves #329.

Introduces a new method fetch_transaction on StandardDatabase that allows users to create a TransactionDatabase based on an existing Transaction, instead of starting a new transaction.

@Moortiii Moortiii force-pushed the feature/fetch-transaction branch from 344867a to 52c214f Compare March 11, 2024 11:20
Copy link
Member

@apetenchea apetenchea left a comment

Choose a reason for hiding this comment

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

Thanks for the prompt followup!
Looks good overall and the logic is correct (also docs example is nice). The only thing which should be addressed is in executor.py, but that's merely a simplification.

@Moortiii Moortiii force-pushed the feature/fetch-transaction branch from 52c214f to 26e22b5 Compare March 11, 2024 14:35
@Moortiii Moortiii requested a review from apetenchea March 11, 2024 14:41
Copy link
Member

@apetenchea apetenchea left a comment

Choose a reason for hiding this comment

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

Change looks good to me!

Unfortunately the docs CI is complaining of duplicate keys. Just use some different names in the example.

@Moortiii Moortiii force-pushed the feature/fetch-transaction branch from 26e22b5 to 75c2c99 Compare March 11, 2024 15:38
@Moortiii Moortiii force-pushed the feature/fetch-transaction branch from 75c2c99 to 9d210e2 Compare March 11, 2024 15:41
@Moortiii
Copy link
Contributor Author

I see that there are some failing checks still, but they don't seem to be related to anything I've touched or something that should have been impacted by my changes.

Copy link
Member

@apetenchea apetenchea left a comment

Choose a reason for hiding this comment

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

The failing docs test is due to the fact that Pregel has been deprecated recently. That's an update we'll have to make on our end.

Thank you for your contribution!

@apetenchea apetenchea merged commit 21f25fe into arangodb:main Mar 11, 2024
51 of 52 checks passed
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.

feature request: Continue an existing transaction
2 participants