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

TestTransaction should implement Transaction instead of TransactionBase #1124

Open
ctubbsii opened this issue Nov 16, 2022 · 3 comments
Open
Labels
good first issue Well written issue that is very specific and easy to do, intended for first time contributors.

Comments

@ctubbsii
Copy link
Member

Currently, TestTransaction implements TransactionBase, but still has a commit() and close() method with the same signature as the Transaction interface, which itself extends AutoCloseable. It should implement Transaction instead.

TestTransaction also has a done() method that does:

try {
  commit();
} finally {
  close();
}

Once TestTransaction is AutoCloseable, inheriting that from Transaction, the calls to done() can, and should, be converted to calls to commit() at the end of a try-with-resources block, and the done() method should be removed. This will make the lifecycle management of test transactions in the test code more clear, rather than relying on the implied close() method inside the done(). It will also make the order of commit() calls more clear when writing/maintaining test code. With commit() hidden inside the done() method, it's not immediately obvious when they are called.

This proposal would cause code written like:

    TestTransaction tx = new TextTransaction();
    // ... stuff here
    tx.done(); // implied commit() and close()

to be written more explicitly as:

    try (TestTransaction tx = new TextTransaction()) {
      // ... stuff here
      tx.commit(); // explicit commit()
    } // auto closed

This example doesn't look that much better, but it's a big improvement when there are multiple transactions being created, committed, and closed in a test.

@ctubbsii ctubbsii added the good first issue Well written issue that is very specific and easy to do, intended for first time contributors. label Nov 16, 2022
@ctubbsii
Copy link
Member Author

Note to potential contributors: this could be done in two smaller steps, as separate PRs.

  1. inline all instances of TestTransaction.done() (and remove the method), and change implements TransactionBase to implements Transaction (adding @Override annotations to close() and commit() would be nice, too), and
  2. track down TestTransaction declarations and convert them to use try-with-resources blocks instead of explicitly calling close()

@555vedant
Copy link

is this issue still active ??

@ctubbsii
Copy link
Member Author

is this issue still active ??

The issue is probably still relevant, as no action has been done to address it, but the project overall has become relatively inactive lately. You are nevertheless welcome to contribute a PR if you wish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Well written issue that is very specific and easy to do, intended for first time contributors.
Projects
None yet
Development

No branches or pull requests

2 participants