kinggoesgaming commented

(Migrated from github.com)

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

#406

**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 commented

2019-07-04 17:58:44 -05:00

(Migrated from github.com)

@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 commented

2019-07-05 05:11:07 -05:00

(Migrated from github.com)

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

kinggoesgaming commented

2019-07-11 18:20:24 -05:00

(Migrated from github.com)

psss.. @KodrAus opinions?

KodrAus commented

2019-07-15 18:39:41 -05:00

(Migrated from github.com)

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)

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 commented

2019-07-16 14:41:45 -05:00

(Migrated from github.com)

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)

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

> 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 #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 commented

2019-07-20 07:33:35 -05:00

(Migrated from github.com)

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.

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: ```rust use uuid::{Uuid, adapter::endian}; let uuid: Uuid = endian::Big::from(my_u128).into(); ``` or ```rust 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 commented

2019-07-21 23:43:50 -05:00

(Migrated from github.com)

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

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 commented

2019-07-28 12:23:36 -05:00

(Migrated from github.com)

@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 commented

2019-07-28 17:05:54 -05:00

(Migrated from github.com)

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'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`](https://docs.rs/uuid/0.7.4/uuid/adapter/compact/index.html) would be the most consistent with how we already do things.

kinggoesgaming commented

2019-07-29 20:21:51 -05:00

(Migrated from github.com)

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 :)

> 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`](https://docs.rs/uuid/0.7.4/uuid/adapter/compact/index.html) would be the most consistent with how we already do things. I think that's a good compromise :)

kinggoesgaming commented

2019-10-10 00:51:51 -05:00

(Migrated from github.com)

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

KodrAus commented

2019-10-10 17:20:42 -05:00

(Migrated from github.com)

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

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Rows
Columns

WIP: add endian adapters #416

Description

Motivation

Tests

Related Issue(s)

Pull request closed