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

Remove database::Builder #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SabrinaJewson
Copy link
Collaborator

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.

Copy link
Collaborator

@wez wez left a 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.

@SabrinaJewson
Copy link
Collaborator Author

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 String and StringBuilder types like some languages do, instead we just have a single mutable String type and you’re expected to know when it is “finished” and not. This sort of goes hand in hand with Rust’s design choice of allowing all variables to be mutable; you could already have made methods that changed the name of a &mut Database, if inconveniëntly, by simply reconstructing the Database with a different name from a different Builder.

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 Arc<Database> or share it between functions it would be immutable by default, eliminating the need for distinctions in most cases).

I don’t think I have any future changes in mind that require this though.

@wez
Copy link
Collaborator

wez commented Apr 1, 2023

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/
http://cliffle.com/blog/rust-typestate/
https://rustype.github.io/notes/notes/rust-typestate-series/rust-typestate-index

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!

@SabrinaJewson
Copy link
Collaborator Author

SabrinaJewson commented Apr 1, 2023

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 globset’s compilation, tempfile’s file creätion etc), those that are a workaround for lack of named parameters (like rustls’s), those for which the built type is mutable anyway (like http::request::Builder), I could only find ariadne::ReportBuilder that is similar to this crate’s builder type, and that’s fairly rare out of the many builders encountered.

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:

  • Users can always use let database = database; to signal that it has become immutable. I am a fan of this pattern, in fact.
  • Read-only fields might be a cool addition to the language…
  • …or in the absence of that, an Immutable<T> wrapper might be useful sometimes.

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 Command builder type. If we are to accept that as a good decision, it would follow that it would be a good decision to add it to our Builder too — but then Builder’s API would just be a superset of Database’s, where users could achieve the same restrictions as Database by using let database = database;. So then it makes more sense to only have Builder, and rename it Database: the same effect as having a mutable Database.

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.

2 participants