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

add endian adapters #416

Closed
wants to merge 5 commits into from
Closed

add endian adapters #416

wants to merge 5 commits into from

Conversation

kinggoesgaming
Copy link
Member

I'm submitting a(n) feature

Description

Add uuid::adapter::endian::{Big, Little}.

Motivation

Currently we have single representation for uuid::Uuid in-memory. However people have needed for little endian. Particularly bindings have a requirement of the memory layout (see #406).

Tests

Currently new tests are needed to be added

Related Issue(s)

#406

@kinggoesgaming
Copy link
Member Author

@uuid-rs/uuid I need your input on this. I want to make sure I am doing this correct before I add serde and other content to this PR

@Dylan-DPC-zz
Copy link
Member

looks good to me but good to get an opinion from @KodrAus as well

@kinggoesgaming
Copy link
Member Author

psss.. @KodrAus opinions?

@KodrAus
Copy link
Member

KodrAus commented Jul 15, 2019

Hmm, I think having types specifically for this might be clumsy to use in practice, because what you really want is a Uuid, rather than a newtype that wraps it.

What if we have two functions: from_u128_be and from_u128_le and either deprecate the From<u128> impl or note in the docs that it's platform-dependent? (personally I'd be leaning towards deprecating because it's a bit of a footgun)

@kinggoesgaming
Copy link
Member Author

Hmm, I think having types specifically for this might be clumsy to use in practice, because what you really want is a Uuid, rather than a newtype that wraps it.

What if we have two functions: from_u128_be and from_u128_le and either deprecate the From<u128> impl or note in the docs that it's platform-dependent? (personally I'd be leaning towards deprecating because it's a bit of a footgun)

  1. From<u128> impl was already removed in remove support for u128 #407
  2. I was thinking that all the _be and _le methods be moved to respective endian adapters. This includes the to_fields_{be,le}.

@KodrAus
Copy link
Member

KodrAus commented Jul 20, 2019

What do we expect a caller to do with a endian::Big or endian::Little? If its only purpose is to act as a point on your way to a Uuid then it seems like the difference between:

use uuid::{Uuid, adapter::endian};

let uuid: Uuid = endian::Big::from(my_u128).into();

or

use uuid::Uuid;

let uuid = Uuid::from_u128_be(my_u128);

But if there's something else these adapters would do then maybe they are worth having as separate structs.

@kinggoesgaming
Copy link
Member Author

One big one I can think is (de)serialization. Especially given that you that uuid is stored in a particular endian and you don't to risk messing up..

That aside I am fine with providing the methods on Uuid itself

@kinggoesgaming
Copy link
Member Author

@uuid-rs/uuid can I have a resolution on which way I should go on this so I can finish this PR then finish the few documentation bits I want to have in this for the release?

@KodrAus
Copy link
Member

KodrAus commented Jul 28, 2019

I'm happy for you to take this in the direction you feel is most appropriate.

Personally, I think a combination of constructor methods on Uuid, and a serialization module like adapter::compact would be the most consistent with how we already do things.

@kinggoesgaming
Copy link
Member Author

I'm happy for you to take this in the direction you feel is most appropriate.

Personally, I think a combination of constructor methods on Uuid, and a serialization module like adapter::compact would be the most consistent with how we already do things.

I think that's a good compromise :)

bors bot added a commit that referenced this pull request Oct 4, 2019
427: Breaking API features and refactoring for `0.8` r=kinggoesgaming a=KodrAus

**I'm submitting a(n)** (|feature|refactor|)

# Description

## Modules and Errors

This PR is a broad refactoring of our error types and module layout. The general pattern I've gone for is:

- Define specific error types in submodules. These are currently private.
- Collect these specific error types in an opaque root `Error`. All methods return this single `Error` type so it's easier for consumers to carry `uuid::Error`s in their own code.

It'll also include some implementations for open PRs as we've been discussing. I imagine we'll want to spend time working through these changes :)

I've hidden our `prelude` module for now, because I'm not sure it's something we'll want to stabilize with (it's only got a few bits in it afterall).

## 128bit support

Re-enables support for 128-bit numbers in the form of constructors on `Uuid`, following the pattern that method names without an endian suffix are BE.

## No-std

Refactors our `no-std` support so we always mark the crate as `no-std` so our std imports are always the same. This simplifies our std/core support so we need fewer modules.

## Timestamps

Includes the design proposed in #405 for timestamp support originally implemented by @jonathanstrong.

# Related Issue(s)

- #406

# Related PR(s)

- #405 
- #416

Co-authored-by: Ashley Mannix <[email protected]>
Co-authored-by: Jonathan Strong <[email protected]>
@kinggoesgaming
Copy link
Member Author

@KodrAus I should close this... since your PR technically superseded this one

@KodrAus
Copy link
Member

KodrAus commented Oct 10, 2019

Ah yes that's right. Thanks @kinggoesgaming, I'll close this one now.

@KodrAus KodrAus closed this Oct 10, 2019
@KodrAus KodrAus deleted the adapter/endian-base branch October 18, 2019 01:55
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.

3 participants