API Refactoring

rrichardson commented

2022-08-17 10:07:22 -05:00

(Migrated from github.com)

Motivation

The UUID Crate has grow organically over the last 5 years. It's seen many new features, and groups of features added, and new uuid version support has been added, where convenient. The Rust ecosystem has evolved significantly in that time. The implementations in the crate have sort of evolved in different directions, there is the Builder, which has functions which are general, and it has functions which are specific to a single UUID version (without mentioning that version's name, see ::from_random_bytes)).

There are about 15 features in this crate (if you include the newest versions). IMO, that number should be closer to 4.

As a result of its growths and offshoots in various areas, the crate is a bit of a mess, with many inconsistencies and some characteristics that make it look a bit dated.

Solution

I propose we refactor the crate in preparation for a new major release.
The basic idea would be this:

We remove all version-specific features, and provide 3 top-level, unified APIs. Both APIs can build any Uuid version, which tools they have to build said versions is up to the included features. The top level APIs would be:
1. A high-level builder API that can build all version
2. A lower-level, module-based API which exposes consistent functions for every version of uuid. There would also be version-specific functions where needed. Where each implementation is in its own module, e.g.
  uuid::v1::new(...)
3. The Uuid struct, which would not know how to construct itself directly, outside of parsing/converting Uuids in other formats.
The only Cargo features would be ones that are specific to target platform restrictions (e.g. wasm, embedded, nostd, fast-random vs /dev/random etc)
We put all stateful and random generation things inside a GeneratorContext (or whatever we want to name it) This way, if people wish to use their own RNGs or clock-sequences, or whatever, it can be tucked neatly away inside that.
Timestamp would stay, but would be abstracted away by the high-level functions, people shouldn't have to generate their own time if they don't want to. (if they disable std, then they would have to bring their own time functions and construct their own timestamps)

If people are OK with this basic arrangement, then I will start working on an RFC.

**Motivation** The UUID Crate has grow organically over the last 5 years. It's seen many new features, and groups of features added, and new uuid version support has been added, where convenient. The Rust ecosystem has evolved significantly in that time. The implementations in the crate have sort of evolved in different directions, there is the Builder, which has functions which are general, and it has functions which are specific to a single UUID version (without mentioning that version's name, see ::from_random_bytes)). There are about 15 features in this crate (if you include the newest versions). IMO, that number should be closer to 4. As a result of its growths and offshoots in various areas, the crate is a bit of a mess, with many inconsistencies and some characteristics that make it look a bit dated. **Solution** I propose we refactor the crate in preparation for a new major release. The basic idea would be this: 1. We remove all version-specific features, and provide 3 top-level, unified APIs. Both APIs can build any Uuid version, which tools they have to build said versions is up to the included features. The top level APIs would be: 1. A high-level builder API that can build all version 2. A lower-level, module-based API which exposes consistent functions for every version of uuid. There would also be version-specific functions where needed. Where each implementation is in its own module, e.g. `uuid::v1::new(...)` 3. The Uuid struct, which would not know how to construct itself directly, outside of parsing/converting Uuids in other formats. 2. The only Cargo features would be ones that are specific to target platform restrictions (e.g. wasm, embedded, nostd, fast-random vs /dev/random etc) 3. We put all stateful and random generation things inside a GeneratorContext (or whatever we want to name it) This way, if people wish to use their own RNGs or clock-sequences, or whatever, it can be tucked neatly away inside that. 4. Timestamp would stay, but would be abstracted away by the high-level functions, people shouldn't have to generate their own time if they don't want to. (if they disable std, then they would have to bring their own time functions and construct their own timestamps) If people are OK with this basic arrangement, then I will start working on an RFC.

KodrAus commented

2022-08-21 22:27:39 -05:00

(Migrated from github.com)

Thanks for the insights @rrichardson! I'd be keen to see what an alternative greenfield API might end up looking like. I'd also be keen to see what real benefits that API offers over the current organization. The current design is that:

Builder can construct any UUID version using whatever tools you have to produce them. This is currently true for v3, v4, and v5 UUIDs where users want to plug in their own implementations for randomness and SHA1/MD5 hashing.
Uuid can hold and format any UUID, but only has high-level APIs to construct them when certain crate features are enabled.

This is a bit of a less obvious division of responsibilities as your proposed API, but is similar conceptually. I'm not currently expecting us to make any breaking changes to the library anytime in the foreseeable future, but also wouldn't want to discourage alternatives that are simply better, so if there's real material benefits we should keep an open mind.

Thanks for the insights @rrichardson! I'd be keen to see what an alternative greenfield API might end up looking like. I'd also be keen to see what real benefits that API offers over the current organization. The current design is that: - `Builder` can construct any UUID version using whatever tools you have to produce them. This is currently true for v3, v4, and v5 UUIDs where users want to plug in their own implementations for randomness and SHA1/MD5 hashing. - `Uuid` can hold and format any UUID, but only has high-level APIs to construct them when certain crate features are enabled. This is a bit of a less obvious division of responsibilities as your proposed API, but is similar conceptually. I'm not currently expecting us to make any breaking changes to the library anytime in the foreseeable future, but also wouldn't want to discourage alternatives that are simply better, so if there's real material benefits we should keep an open mind.

rrichardson commented

2022-08-21 22:51:18 -05:00

(Migrated from github.com)

The most practical immediate benefit of reducing the number of features down from 15 to 4 would be that two of the CI jobs will work again (they run their container out of disk space with the powerset of features (I'm sure there are easier ways to address this :) ))

I think the API would not change much, I think, but the salient points are:

The UUID versions should no longer be locked by features, however, some of the necessary bits to build some versions would need to be supplied by the user if they opt out of the necessary components.

The best way I can think of to offer a unified interface over the variety of required units is with a builder API.

Also, I think the context should be required for either counters/sequences or rng's. I would prefer that I manage the state that such things requires.

My preference for the timestamp is just the opposite. I'd prefer that the API abstract that away from me.

The most practical immediate benefit of reducing the number of features down from 15 to 4 would be that two of the CI jobs will work again (they run their container out of disk space with the powerset of features (I'm sure there are easier ways to address this :) )) I think the API would not change much, I think, but the salient points are: The UUID versions should no longer be locked by features, however, some of the necessary bits to build some versions would need to be supplied by the user if they opt out of the necessary components. The best way I can think of to offer a unified interface over the variety of required units is with a builder API. Also, I think the context should be required for either counters/sequences or rng's. I would prefer that I manage the state that such things requires. My preference for the timestamp is just the opposite. I'd prefer that the API abstract that away from me.

KodrAus commented

2022-08-22 00:39:54 -05:00

(Migrated from github.com)

two of the CI jobs will work again (they run their container out of disk space with the powerset of features (I'm sure there are easier ways to address this :) ))

That's a fair observation. The cost of confident builds with so many possible combinations is a burden from feature flag creep I've hit in other projects too. One of the reasons we have the features is that some users really want to avoid pulling in additional package dependencies wherever possible, and for a fundamental utility it seemed reasonable to continue to try cater to them.

> two of the CI jobs will work again (they run their container out of disk space with the powerset of features (I'm sure there are easier ways to address this :) )) That's a fair observation. The cost of confident builds with so many possible combinations is a burden from feature flag creep I've hit in other projects too. One of the reasons we have the features is that some users really want to avoid pulling in additional package dependencies wherever possible, and for a fundamental utility it seemed reasonable to continue to try cater to them.

KodrAus commented

2022-09-28 23:50:41 -05:00

(Migrated from github.com)

I’ve been running back over things in #625 and I definitely see your point. I’ve tried to paper over some of the issues a bit, but we could do a lot better.

Rows
Columns

API Refactoring #612