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 fields on InsertOption, UpdateOption, etc public #71

Closed
ManevilleF opened this issue Mar 16, 2021 · 3 comments
Closed

Make fields on InsertOption, UpdateOption, etc public #71

ManevilleF opened this issue Mar 16, 2021 · 3 comments

Comments

@ManevilleF
Copy link
Contributor

enums like:

  • InsertOptions
  • UpdateOptions
  • ReplaceOptions
  • ...

all have their fields being private and we have to use the derived TypedBuilder for customization.

I have an issue with that, in some cases I want to leave a None value which is not possible in current implementation.

Example:

let builder = InsertOptions::builder();
if some_condition {
   builder.wait_for_sync(true);
}
builder.build()

So I have two proposals:

  • Removing the TypedBuilder implementations and making a custom builder system
  • Making the fields public allowing builder and custom

Either way I can make a MR for this

@fMeow
Copy link
Owner

fMeow commented Mar 28, 2021

It is actually possible. The point is that TypedBuilder returns a new builder with a different type.

let builder = InsertOptions::builder();
let option = if some_condition {
   builder.wait_for_sync(true).build()
}
else{
   builder.build()
};

The pros of TypedBuilder is that we can know if the code is wrong at compile time and achieve zero cost abstraction. But I do think it a little cubersome in this case that you have to use a single match for all the conditions you want.

We can also create a builder for each condition:

let option = if some_condition {
   InsertOptions::builder().wait_for_sync(true).build()
}
else{
   InsertOptions::builder().build()
};

@ManevilleF
Copy link
Contributor Author

ManevilleF commented Apr 2, 2021

It is actually possible. The point is that TypedBuilder returns a new builder with a different type.

let builder = InsertOptions::builder();
let option = if some_condition {
   builder.wait_for_sync(true).build()
}
else{
   builder.build()
};

I tried this and it doesn't work. The second option works but I put a lof of options and My code looks like this:

impl Into<UpdateOptions> for OperationOptions {
    fn into(self) -> UpdateOptions {
        // TODO: remove this builder pattern whe arangors resolves https://github.com/fMeow/arangors/issues/71
        if let Some(value) = self.wait_for_sync {
            UpdateOptions::builder()
                .keep_null(true)
                .wait_for_sync(value)
                .ignore_revs(self.ignore_revs)
                .return_new(true) 
                .return_old(false)
                .silent(false)
                .build()
        } else {
            UpdateOptions::builder()
                .keep_null(true)
                .ignore_revs(self.ignore_revs)
                .return_new(true)
                .return_old(false)
                .silent(false)
                .build()
        }
    }
}

(source: https://gitlab.com/qonfucius/aragog under src/db/operation_options.rs)

@ManevilleF
Copy link
Contributor Author

I tried again and it works

It is actually possible. The point is that TypedBuilder returns a new builder with a different type.

let builder = InsertOptions::builder();
let option = if some_condition {
   builder.wait_for_sync(true).build()
}
else{
   builder.build()
};

The pros of TypedBuilder is that we can know if the code is wrong at compile time and achieve zero cost abstraction. But I do think it a little cubersome in this case that you have to use a single match for all the conditions you want.

We can also create a builder for each condition:

let option = if some_condition {
   InsertOptions::builder().wait_for_sync(true).build()
}
else{
   InsertOptions::builder().build()
};

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

No branches or pull requests

2 participants