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

Make IsolationLevel pub #292

Merged
merged 2 commits into from
Apr 18, 2024
Merged

Make IsolationLevel pub #292

merged 2 commits into from
Apr 18, 2024

Conversation

Dhghomon
Copy link
Contributor

@Dhghomon Dhghomon commented Jan 31, 2024

Resolves #291 in which a user noticed that IsolationLevel is required for TransactionOptions::isolation but didn't make the pub use statement for the rest of the types in the options module.

@Dhghomon Dhghomon changed the title Make isolationlevel pub Make IsolationLevel pub Jan 31, 2024
@Dhghomon Dhghomon marked this pull request as ready for review January 31, 2024 23:48
@Dhghomon Dhghomon requested review from elprans and fantix January 31, 2024 23:49
@elprans
Copy link
Member

elprans commented Feb 1, 2024

I think we should just remove/unpublish the isolation method. It's not useful, because serializable is the only isolation option.

@Dhghomon
Copy link
Contributor Author

Dhghomon commented Feb 1, 2024

I think we should just remove/unpublish the isolation method. It's not useful, because serializable is the only isolation option.

That would work. Should we remove the IsolationLevel enum as well or are there plans to add to it in the future?

Dhghomon and others added 2 commits April 18, 2024 17:25
At the moment, isolation level of a transaction can only be
SERIALIZABLE. It can be specified when opening a connection, but
if it is not, it will default to SERIALIZABLE.

There is also no plans to support other level soon.

IsolationLevel thus serves no purpse and also cannot be set outside
of this crate, since it is not public.

So there is no harm in removing it. When/if we implement other
levels, we can revert this commit.
@aljazerzen aljazerzen force-pushed the make-isolationlevel-pub branch from 1efdbfa to 57f6771 Compare April 18, 2024 15:33
@aljazerzen
Copy link
Contributor

I've removed isolation method and IsolationLevel itself. Here is justification from commit message:

At the moment, isolation level of a transaction can only be
SERIALIZABLE. It can be specified when opening a connection, but
if it is not, it will default to SERIALIZABLE.

There is also no plans to support other level soon.

IsolationLevel thus serves no purpse and also cannot be set outside
of this crate, since it is not public.

So there is no harm in removing it. When/if we implement other
levels, we can revert this commit.

@aljazerzen aljazerzen requested a review from quinchs April 18, 2024 15:34
@aljazerzen aljazerzen merged commit 4d929c7 into master Apr 18, 2024
4 checks passed
@aljazerzen aljazerzen deleted the make-isolationlevel-pub branch April 18, 2024 16:50
@CodesInChaos
Copy link
Contributor

If you later re-add it (e.g. because you implement snapshot readonly support), you should mark the enum as #[non_exhaustive], so adding more options isn't a breaking change.

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.

Export IsolationLevel
5 participants