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

fix(foundationdb): make the db_run non capturing #113

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

Akanoa
Copy link
Contributor

@Akanoa Akanoa commented Jan 11, 2024

Hello 👋

I would like to introduce a modification in the Database::run parameters signature.

The function is great, but there is a little flaw with the maybe_committed closure parameter. As this one is a boolean which is "copiable".

Therefore, if you want to use this parameter, you have to make the closure capturing.

db.run(|trx, maybe_committed| async move {
    if maybe_committed {
        // treatment
    }
    Ok(())
})
.await

But sometimes this behavior is not desired. For example, when, your closure uses an immutable reference of a variable in the outer environment.

#[derive(Debug)]
struct Data;
let data = Data;

db.run(|trx, maybe_committed| async move {
    if maybe_committed {
        // treatment
    }
    dbg!(&data)
    Ok(())
})
.await

With the error:

cannot move out of `data`, a captured variable in an `Fn` closure

There are two ways to making it functional

By using an Arc

#[derive(Debug)]
struct Data;

let data = Arc::new(Data);

db.run(|trx, maybe_committed| {
    let data = data.clone();

    async move {
        if maybe_committed {
            // treatment
        }
        dbg!(&data);
        Ok(())
    }
})
.await

Which wraps the variable in an immutable state.

Or by forcing the move of the boolean instead of its copy. This is done by wrapping the boolean into a non-copy structure.

The boolean can be recovered through the into() call

#[derive(Debug)]
struct Data;

let data = Data;

db.run(|trx, maybe_committed| {
    struct Wrapper(bool);
    impl From<Wrapper> for bool {
        fn from(value: Wrapper) -> Self {
            value.0
        }
    }

    let maybe_committed = Wrapper(maybe_committed);

    async {
        if maybe_committed.into() {
            // treatment
        }
        dbg!(&data);
        Ok(())
    }
})
.await

This way the data keeps its type.

As I prefer the second solution, but because the boilerplate is a bit heavy. I would like to modify the boolean type of mayby_committed into a wrapped boolean implementing Into.

This way the API becomes:

db.run(|trx, maybe_committed| async {
    if maybe_committed.into() {
        // treatment
    }
    dbg!(&data);
    Ok(())
})
.await

Leaving the closure non-capturing.

@Akanoa Akanoa marked this pull request as ready for review January 11, 2024 18:07
@codecov-commenter
Copy link

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (a19d710) 82.37% compared to head (5554f18) 83.04%.

Files Patch % Lines
foundationdb/src/database.rs 0.00% 6 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #113      +/-   ##
==========================================
+ Coverage   82.37%   83.04%   +0.66%     
==========================================
  Files          23       23              
  Lines        4909     4914       +5     
==========================================
+ Hits         4044     4081      +37     
+ Misses        865      833      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PierreZ PierreZ merged commit a0d1a55 into foundationdb-rs:main Jan 16, 2024
30 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.

None yet

3 participants