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

Fix extra fixes / cleanups for object IDs #231

Merged
merged 4 commits into from
Aug 21, 2019
Merged

Conversation

daboross
Copy link
Collaborator

Looking over the changes made in #229, I realized that using u128 more could eliminate most/all of the manual formatting code for RawObjectId, and that a few more transitions which might be useful were missing.

This:

  • adds impls TryFrom<u128>, Into<u128>, From<[u32; 3]>
  • fixes the MAX_PACKED_VAL constant, previously off by 1
  • removes manual formatting code in favor of calling using u128's code

This adds some more conversions between ObjectId and another binary
format which it might be useful to take.

This also takes advantage of u128 formatting code, and uses it instead
of manually formatting the three integers as one.
@@ -250,6 +286,29 @@ mod test {
}
}

#[test]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following @Dessix suggestion, it might be worth testing on wasm.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe CI should be running all plain #[test] tests on WASM too as of a1d2162.

It doesn't seem to indicate what tests succeed, but a cursory test shows that locally, cargo web test --verbose --nodejs will fail if I insert a manual panic in here.

@daboross daboross merged commit 02f6c05 into master Aug 21, 2019
@daboross daboross deleted the extra-object-id-changes branch August 21, 2019 04:51
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