Skip to content

RUST-314 Remove constants for subtypes and element types #152

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

Merged
merged 1 commit into from
Apr 7, 2020

Conversation

saghm
Copy link
Contributor

@saghm saghm commented Apr 3, 2020

No description provided.

@saghm saghm marked this pull request as ready for review April 3, 2020 19:47
@saghm saghm requested a review from patrickfreed April 3, 2020 19:47
const BINARY_SUBTYPE_BINARY_OLD: u8 = 0x02;
const BINARY_SUBTYPE_UUID_OLD: u8 = 0x03;
const BINARY_SUBTYPE_UUID: u8 = 0x04;
const BINARY_SUBTYPE_MD5: u8 = 0x05;

/// All available BSON element types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While looking at this, it got me thinking that we might want to revisit the names used here and in Bson.

A couple of things come to mind:

  • FloatingPoint: when surveying other drivers, all but Ruby (which doesn't have a explicit type for Bson but instead extends existing Float) refer to this as "Double". I know there isn't a double type in Rust, but the same is true for Go, and they still refer to the BSON type as a "double".
  • NullValue can probably be just Null like the Bson enum case
  • Integer32Bit / Interger64Bit / Decimal128Bit can be Int32 / Int64 / Decimal128 respectively
    • Usually I'm for verbosity over terseness, but in this case the verbosity actually hurts discoverability imo
    • Given we're in Rust, maybe it would make sense to use I32 / I64 like the Bson enum case names
  • TimeStamp I think is usually just Timestamp with no capital "S"

One other oddity is that for strings, the Bson enum case is String and the type is Utf8String, but for datetimes, the Bson case and type case are both UtcDatetime. The UTC requirement is expressed at the type level by defining the case's associated value as a DateTime<Utc>, so the "Utc" prefix is redundant and, imo, can be dropped.

All this feedback isn't exactly related to the ticket here, so I'm fine with not addressing it here. I just figured this was a decent spot to mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this ticket is removing them from the public API, I'm not overly concerned about the names of these constants as long as they're easy to understand. I'm not strictly opposed to renaming the Bson enum variants though; want to file a ticket for that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah by "here" I actually meant the ElementType enum below. Filed RUST-345.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry for misunderstanding. Thanks for filing the ticket!

@saghm saghm merged commit 9055e18 into mongodb:master Apr 7, 2020
@saghm saghm deleted the RUST-314 branch April 7, 2020 15:28
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.

2 participants