search

dyedgreen / deno-sqlite

Deno SQLite module
https://deno.land/x/sqlite
MIT License
409 stars 36 forks source link

WIP: Add API to determine the transaction state #220

Closed rherrmann closed 1 year ago

rherrmann commented 1 year ago

Add a transactionState property to DB that returns the current state.

I'd appreciate guidance how to proceed if you would consider this for inclusion

Does evaluating two admittedly related SQLite functions into a single transactionState (or txnState?) align with the technical direction or would you prefer separate properties in DB?

Should generated files be included in this commit? I think I've seen a GitHub Action but I'm unsure how/when to trigger it.

dyedgreen commented 1 year ago

I think these feel separate enough that we should have different functions for them.

Something similar to the following could work well:

class DB {

    // ...

    transactionState(schema? = "main"): "none" | "read" | "write"

    // ...

    get autoCommit(): boolean;
}

I'm unsure if we should use string constants to represent the transaction state (or go the same way as the Status codes we expose on errors and define a variant).

Since these should probably be split across separate PRs, I think we can go ahead with a separate PR to simply expose the autoCommit property and then think about how to encode the transaction state return type.

Should generated files be included in this commit?

This is not necessary; the CI tests re-built the artifacts regardless, and once a change is merged into the main branch, the generated files will be updated automatically.

rherrmann commented 1 year ago

I've implemented your suggestion for autoCommit in https://github.com/dyedgreen/deno-sqlite/pull/221.

Once it is accepted, I can get back to this when I the find time and rework the change to add a simple transactionState. Exact type for the state is yet to be decided.

rherrmann commented 1 year ago

I'm afraid, I won't get to this in the foreseeable future.