Skip to content

Improve macro syntax support #83

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 13 commits into from
Oct 2, 2017
Merged

Improve macro syntax support #83

merged 13 commits into from
Oct 2, 2017

Conversation

kyeah
Copy link
Contributor

@kyeah kyeah commented Oct 1, 2017

What

Overhauls the bson/doc macro definitions to support syntactic sugar available in the json! macro:

  • colon-separated key-value pairs
  • Non-parenthesized values with . characters
  • Trailing commas
  • null keyword

How

By taking serde's json! macro and making a couple changes (including overloaded support for rocket separators for backwards-compatibility), and some simplifications (removing redundant boolean cases, etc). I ran the mongo driver tests with this branch of bson and they all passed, which is convincing enough for me (along with our bson-rs tests) that we don't have any regressions.

Who

@zonyitoo, @saghm

Copy link
Contributor

@saghm saghm left a comment

Choose a reason for hiding this comment

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

I only glossed over the macro implementation itself, as it's not worth blocking this for the amount of time it would take me to actually understand all of it

@@ -397,19 +397,19 @@ impl Bson {
match *self {
Bson::RegExp(ref pat, ref opt) => {
doc! {
"$regex" => (pat.clone()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm so confused about git saying the first ')' was removed here but the second below

src/bson.rs Outdated
@@ -397,19 +397,19 @@ impl Bson {
match *self {
Bson::RegExp(ref pat, ref opt) => {
doc! {
"$regex" => (pat.clone()),
"$options" => (opt.clone())
"$regex" => pat.clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, why remove the parens/add trailing commas but not switch the "=>" to ":"? Is this a rustfmt thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, just made these changes before I re-added colon support. i'll change these.

/// #
/// # fn main() {
/// let value = bson!({
/// "code": 200,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to document the "=>" syntax, or are we considering that discouraged (but kept around for compatibility)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering it discouraged in favor of consistency, but here for backwards-compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good

#[macro_export]
macro_rules! bson {
([]) => {{ $crate::Bson::Array(Vec::new()) }};
//////////////////////////////////////////////////////////////////////////
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume these comments are also lifted from serde_json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah; not sure where the line falls in terms of fair usage.

tests/lib.rs Outdated
"doc" => {
"fish" => "in",
"a" => "barrel",
"!" => 1
"!" => 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the trailing comma required? If not, we should probably add a separate test for without it (both nested and top-level)

tests/lib.rs Outdated
"doc": {
"fish": "in",
"a": "barrel",
"!": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto above about additional tests for without trailing commas

@zonyitoo
Copy link
Contributor

zonyitoo commented Oct 2, 2017

LGTM

@kyeah kyeah merged commit 36127d8 into master Oct 2, 2017
@kyeah kyeah deleted the kev_macro-improvements branch October 2, 2017 13:50
@kyeah
Copy link
Contributor Author

kyeah commented Oct 2, 2017

@zonyitoo ready for a publish. thanks 🙏

@zonyitoo
Copy link
Contributor

zonyitoo commented Oct 2, 2017

Published v0.10.0

lrlna pushed a commit to lrlna/bson-rs that referenced this pull request Feb 27, 2019
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