-
Notifications
You must be signed in to change notification settings - Fork 15
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
Remove database::Builder
#36
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the nice properties of the builder pattern is that all the mutation happens in the builder, then the resultant built object is immutable and has a different type.
That separation of mutability helps to guard against eg: passing a partially constructed object around to functions that expect a fully constructed object, because the compiler will type check and catch it.
Is that strictly necessary here? No, but I think it's worth considering this aspect.
With my pragmatic refactoring hat on: this commit feels pretty neutral in terms of lines of code and readability; it doesn't feel like there is a significant gain in terms of maintainability, and the "loss" of the immutable Database
type feels like it weighs more than those gains.
So, I'm very slightly against this change, but if you see this helping some other work you have in mind, I'm not going to block it.
Hmm, I see what you mean. My perspective here is that “the Rust philosophy” is to not really make that distinction — for example, we don’t have separate For those reasons, my intuition has always been against this design, regarding it as a relic of OO where things have identity as “objects” instead of “data”, and the lack of a borrow-checker makes immutability more of a concern (in Rust if you were to construct an I don’t think I have any future changes in mind that require this though. |
FWIW, it's fairly common to see Rust crates adopt this kind of typestate. It's less about pure immutability (which is less critical in Rust, due to the borrow checker) than it is about signaling that an object is ready to be consumed in a particular context and having the compiler surface that class of bug so that our human brains don't have to work so hard. The builder pattern is an example of a subset of typestate programming. Here are a number of blogs that go into more detail about this: https://yoric.github.io/post/rust-typestate/ I'm not trying to win you over in this particular instance, just wanted to share some context on why there can be value in this approach! |
I do know and am a fan of typestate; my point was mainly that immutability is not something that is usually used for typestate, since having “mutable” and “immutable” phases of an object is not, as it stands, the mode of thought in Rust (Rust more considers mutable and immutable phases of values, which is enforced by the borrowchecker and isn’t helped by typestate). Of the builders I could find via my browser history, excluding those that actually do stuff when built (like I guess I just don’t feel that putting non-strictly-necessary unorthodox restrictions on the user is that worth it. Or rather, it might be better for this problem to be solved generically:
I guess my main point is the overall problem spreads in all code, and is out of scope for this crate. Edit: Another precedent against this is in the standard library, where they recently added getter functions to the |
A simpler (in my opinion) design here is to have the database just have its name and description as public fields; the builder pattern seems a bit excessive.