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

Bson: Adds std.uuid.UUID support #1404

Merged
merged 4 commits into from Nov 5, 2017

Conversation

Projects
None yet
3 participants
@denizzzka
Contributor

denizzzka commented Feb 5, 2016

No description provided.

@denizzzka denizzzka changed the title from Adds std.uuid:UUID support to Adds std.uuid.UUID support Feb 5, 2016

@denizzzka denizzzka changed the title from Adds std.uuid.UUID support to Bson: Adds std.uuid.UUID support Feb 5, 2016

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2016

Member

Do you happen to know the binary format that is required for the new UUID type 0x4? The bson module still uses the old code 0x3, which seems to be defined as an application specific representation (so this PR is fine in principle), but I'm wondering if we shouldn't instead directly opt for 0x4 instead of producing legacy data that results in possibly non-interoperable database contents.

Member

s-ludwig commented Feb 10, 2016

Do you happen to know the binary format that is required for the new UUID type 0x4? The bson module still uses the old code 0x3, which seems to be defined as an application specific representation (so this PR is fine in principle), but I'm wondering if we shouldn't instead directly opt for 0x4 instead of producing legacy data that results in possibly non-interoperable database contents.

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 10, 2016

Contributor

Plan for this PR:

  • Remove conversion of std UUIDs to old 0x3 type
  • Add conversion of std UUID to new 0x4 type
  • Both 0x3 and 0x4 types of Bson UUIDs should be readable to std UUIDs

Sounds good?

Contributor

denizzzka commented Feb 10, 2016

Plan for this PR:

  • Remove conversion of std UUIDs to old 0x3 type
  • Add conversion of std UUID to new 0x4 type
  • Both 0x3 and 0x4 types of Bson UUIDs should be readable to std UUIDs

Sounds good?

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 10, 2016

Contributor

Or just merge it, and after we can insensibly make changes in the future.

Contributor

denizzzka commented Feb 10, 2016

Or just merge it, and after we can insensibly make changes in the future.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2016

Member

If you are willing/have the time to do the former, I'd definitely prefer that, but since there are at least no backwards compatibility issues with doing the latter first, I see no real reason to generally rule it out either. The only downside is that it generates data with an obsolete format.

Member

s-ludwig commented Feb 10, 2016

If you are willing/have the time to do the former, I'd definitely prefer that, but since there are at least no backwards compatibility issues with doing the latter first, I see no real reason to generally rule it out either. The only downside is that it generates data with an obsolete format.

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 10, 2016

Contributor

Probably, not obsolete, just old.

Contributor

denizzzka commented Feb 10, 2016

Probably, not obsolete, just old.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 10, 2016

Member

Ok, let's say "deprecated" ;)

Member

s-ludwig commented Feb 10, 2016

Ok, let's say "deprecated" ;)

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 10, 2016

Contributor

It is better than nothing

Also, I am interested in 0x4 UUID support and will implement it later. I am promise! :з

Contributor

denizzzka commented Feb 10, 2016

It is better than nothing

Also, I am interested in 0x4 UUID support and will implement it later. I am promise! :з

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Feb 14, 2016

Contributor

It is need to add also toJson conversion for Bson UUIDs?

Contributor

denizzzka commented Feb 14, 2016

It is need to add also toJson conversion for Bson UUIDs?

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Feb 15, 2016

Member

Hm, right, I didn't think about this. It would definitely be nicer to emit a normal UUID string instead of base64 encoding the binary representation. It also needs to be handled in BsonSerializer (writeValueH, readValue and getBsonTypeID).

Member

s-ludwig commented Feb 15, 2016

Hm, right, I didn't think about this. It would definitely be nicer to emit a normal UUID string instead of base64 encoding the binary representation. It also needs to be handled in BsonSerializer (writeValueH, readValue and getBsonTypeID).

@WebFreak001

This comment has been minimized.

Show comment
Hide comment
@WebFreak001

WebFreak001 Oct 31, 2017

Contributor

@s-ludwig I think that such a change could be done in a separate PR, no need to let this rot for another year

Contributor

WebFreak001 commented Oct 31, 2017

@s-ludwig I think that such a change could be done in a separate PR, no need to let this rot for another year

@denizzzka

This comment has been minimized.

Show comment
Hide comment
@denizzzka

denizzzka Oct 31, 2017

Contributor

PR valid and can be merged, CI accidentally failed and can be restarted.

Contributor

denizzzka commented Oct 31, 2017

PR valid and can be merged, CI accidentally failed and can be restarted.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Nov 5, 2017

Member

Okay thanks! Makes sense as a step forward.

Member

s-ludwig commented Nov 5, 2017

Okay thanks! Makes sense as a step forward.

@s-ludwig s-ludwig merged commit 9a8b026 into vibe-d:master Nov 5, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment