For a postgres-specific crate like postgres_money, these impls would just live directly in that crate and not require a feature over here.

@sfackler thepostgres_money crate is not dependent on therust-postgres crate (nor theToSql andFromSql traits) in order to be useful. It is just representation of data that should mirror what Postgres does for themoney type. In fact, thepostgres_money crate could potentially be valuable without every communicating with Postgres if you wanted to use it to see how Postgres would parse and represent strings (though this is admittedly farfetched).

The pattern I believe you are suggesting is keep all of the impls in the postgres_money crate, but that seems to lead to the following situation:

• Another crate (new_postgres_reader) wants to usepostgres_money
• They open a pull request topostgres_money that implements the traits thatnew_postgres_reader needs to send data to PG
• postgres_money now has an additional dependency on another crate, which has a dependency onpostgres_money. Admittedly easy to feature flag, but still.

This seems to fly in the face of encapsulation, where this crate now has to have knowledge of its consumers, instead of the consumers taking a dependency on this crate. It seems that unless every crate that interfaces with Postgres data directly has the same APIs and signatures, implementing the SQL representation traits forpostgres_money::Money would make sense to live closer to the Postgres binding crate.

As a concrete example, if we pretend Diesel didn't have theMoney type support in place, they could take a dependency onpostgres_money and implement their bindings betweenpostgres_money::Money and their DB interface. Conversely, if we added the binding in thepostgres_money crate,postgres_money would take a dependency on Diesel and vice versa.

So it seems like the SQL-interface layer is better served as a feature within the crates that access Postgres, not within the generic representation of that data.

They open a pull request to postgres_money that implements the traits that new_postgres_reader needs to send data to PG

Don't blame me if I am wrong(a noob here) but isn't postgres_money meant to be more like sattelite crate - an extension for main postgres crates instead? Not like let-everybody-in club.

This seems to fly in the face of encapsulation.

The pattern I believe you are suggesting is keep all of the impls in the postgres_money crate, but that seems to lead to the following situation:

This does not appear to have been a problem in the last 5 or so years of the development of this crate.

You can always have a feature flat on postgres_money to make the dependency on postgres_types optional.

If I were someone new coming to the rust-postgres crate and I wanted support for both theuuid andmoney column types, I would find it odd that one was natively supported (behind a feature) and I'd have to find my own implementation for the other.

If the goal is to have developers choose their own implementation becauserust-postgres doesn't support it natively, then implementing the ToSql and FromSql elsewhere seems reasonable, just not what I had in mind.

I don't understand - you have to use an external implementation of theUuid type just as you would for theMoney type. The features just add the impl for crates where it doesn't make sense for them to support it natively.

I think the drivers for my thought process are basically three things, broken down intouser concerns (devs using the rust-postgres crate) andmaintainer concerns (e.g. you for rust-postgres and me for postgres_money):

User Concerns

Discoverability

As a developer, I decide to us PG as a backend. I discover this crate, and pull it in as a dep. I know I have some tables that will depend onmoney, so I look throughrust-postgres to see how I can interface with this type.

Possible outcomes:

• Ifmoney is a supported feature inrust-postgres, I simply enable it, rebuild and am on my way.
• Ifmoney is not supported, possiblyrust-postgres suggests/documents thepostgres_money crate, the dev can go out and import this crate themselves with the appropriate features. (From my perspective, why wouldrust-postgres suggest a crate but not implement it behind a feature?)
• Ifmoney is not supported and not suggested/documented, the developer is left with the task of finding their own implementation/solution

Of note: this was basically the process I went through when I realized there was nomoney support. It would have been amazing if I could have simply enabled a feature and kept coding.

Robust Postgres Support

This crate saysPostgreSQL support for Rust. There is a gap though - the Postgresmoney type. If this crate is supporting Postgres, why should support for this native Postgres feature not live inside this crate? If themoney support is behind a feature flag, it's hard for me to understand what the drawbacks of collocating money support with a postgres crate are.

You mentioned this hasn't been a concern with other crates - can you give me examples of other crates that implement Postgres native types and also implement rust-postgres ToSql and FromSql impls?

Maintainer Concerns

Maintainability

If your concern is to avoid adding more dependencies to the rust-postgres footprint, or concerns with being the one responsible to maintain the ToSql and FromSql implementations for thepostgres_money crate, that seems like a fine reason not to want to include this dependency. If that is the case, I'm happy to commit to handling bugfixes and feature requests for this bit of functionality.

• Ifmoney is a supported feature inrust-postgres, I simply enable it, rebuild and am on my way.

No, you enable the feature on the postgres dependency and add the dependency on the postgres_money crate. How is that significantly different from adding a dependency on the postgres_money crate and (maybe) enabling a feature on the postgres_money crate?

How is that significantly different from adding a dependency on the postgres_money crate and (maybe) enabling a feature on the postgres_money crate?

Because of discoverability. The developer has already found therust-postgres crate, so enabling a feature within that crate is a matter of reading the docs.

On the other hand, if support formoney is something thatrust-postgres doesn't know about (because it only lives as a feature withinpostgres_money), then the developer must 1) go do research and determine whether or not there aremoney implementations forrust-postgres and 2) spend time evaluating which implementation they want to use.

This is all in addition to the fact thatrust-postgres technically has a feature gap, which is support for Postgres' nativemoney type. It seems like ifrust-postgres is intended as a fully-featured driver for Postgres, support for themoney type would be welcome.

I'm having a hard time understanding your motivations and responses from the feedback you are providing. Perhaps you could share what your goals are for suggesting this live outside of rust-postgres, or what you are optimizing for generally? If you do, I could help address those concerns or understand why my suggested approach is not preferable.

The developer has already found therust-postgres crate, so enabling a feature within that crate is a matter of reading the docs.

The docs can point people towards postgres-money.

I'm having a hard time understanding your motivations and responses from the feedback you are providing.

I want to minimize the number of feature flags exposed by the postgres crate. If a library is built specifically for postgres, like postgres-array, or postgres-range, or postgres-money, then that's great since a feature flag won't be necessary for those types.

I want to avoid having types with nontrivial APIs (like ranges or arrays or money) in this crate. That allows those APIs to have more freedom to iterate through e.g. breaking changes. I do not want to cut breaking releases of the core postgres crates very often since each break pushes work downstream to update.

@sfackler understood. The minimization of feature flags is definitely not what I'd expected, so that is good to know.

I will close this PR. I'd probably lean towards keeping the issue open, but I'll defer to you on that.

Bothuuid (for example) andmoney are built in types to Postgres, as a user I would fully expect to enable support for them in the same way with this crate (assuming they aren't enabled by default). If there's an issue of too many feature flags, perhaps that could be helped by making groups of feature flags? What is the problem with having a lot of feature flags?