Add test_example_59 #1143

kkloberdanz · 2022-11-09T19:14:16Z

Implement a snapshot query example for the manual.

Test skeleton contains comment of Python example code for what this example should perform.

src/libmongoc/tests/test-mongoc-sample-commands.c

kevinAlbs · 2022-11-14T16:19:06Z

src/libmongoc/tests/test-mongoc-sample-commands.c

+      value = bson_iter_value (&iter);
+      if (value->value_type == BSON_TYPE_INT32) {
+         *accumulator += value->value.v_int32;
+      } else if (value->value_type == BSON_TYPE_INT32) {
+         *accumulator += value->value.v_int64;
+      } else {
+         MONGOC_ERROR ("%s", "'adoptableCount' must be an integer");
+         rc = false;
+         goto cleanup;
+      }


Suggest using bson_iter_as_int64 to handle multiple possible numeric types.

Suggested change

value = bson_iter_value (&iter);

if (value->value_type == BSON_TYPE_INT32) {

*accumulator += value->value.v_int32;

} else if (value->value_type == BSON_TYPE_INT32) {

*accumulator += value->value.v_int64;

} else {

MONGOC_ERROR ("%s", "'adoptableCount' must be an integer");

rc = false;

goto cleanup;

}

*accumulator += bson_iter_as_int64 (&iter);

src/libmongoc/tests/test-mongoc-sample-commands.c

kevinAlbs · 2022-11-14T16:22:51Z

Looking good. Include CDRIVER-4295 in the commit message before merging. That will result in a comment with the commit being added to the Jira ticket.

galon1

LGTM! I left one comment about adding more explanation in one part of the example, but I don't think it should block merging if the added explanation is unnecessary.

galon1 · 2022-11-14T17:10:58Z

src/libmongoc/tests/test-mongoc-sample-commands.c

+
+   accumulate_adoptable_count (cats_collection, &adoptable_pets_count);
+   accumulate_adoptable_count (dogs_collection, &adoptable_pets_count);
+


It might be helpful to add a comment here describing that we are using an aggregation pipeline to return the count, since other drivers have been displaying the aggregation pipeline command used. The comment could also be describing how the aggregation pipeline command is omitted.

I'm glad you brought this up, because that's something I'm curious about. It would be a fair amount of duplicate code and a bit lengthy to inline both calls to accumulate_adoptable_count in this function, but I'm wondering if I should do one of the following:

Include the entire function accumulate_adoptable_count in the example, i.e., place the start example comment just above the declaration of accumulate_adoptable_count, and end example comment stays where it is within test_example_59.

Place the text of accumulate_adoptable_count in a comment in test_example_59.

Explain in a comment what is happening in the function accumulate_adoptable_count purely in English, and omit code in the comment.

Inline all the code from accumulate_adoptable_count within test_example_59.

I'm leaning towards 1., but I'm not sure how that would look when rendered. I'd like to know what everyone thinks about this, since I'm not sure which way would be the cleanest for the purpose of producing an example.

P.S. I'm planning on reaching out to Jeffrey Allen, the reporter for the original ticket, regarding this before merging.

I think it would be the most helpful to include the entire function, but since that is many more lines of code than the Python example, we could do option 3 and just describe the aggregation pipeline run. There are other pages (like this one in the C documentation that walks through writing an aggregation pipeline, so it might not be necessary to show every single step in accumulate_adoptable_count .

Good point. One detail that would be good to include is that the client session must be appended to the options passed to mongoc_collection_aggregate. If we go with option 3., then I suppose that could be explained also in a comment, but it seems like showing the code for it would be the best.

I got some feed back from Jeffery Allen. It sounds like he's in favor of option 3.

(quote from Jeff)

Hey Kyle, thanks for reaching out. So my recollection with that ticket is that the main goal is to emphasize the snapshot functionality in the example. Therefore, I'm inclined to say that we can exclude the agg pipeline from the example, and leave a comment explaining the aggregation. That way, users aren't too overwhelmed by a large example and they'll still be able to see the snapshot behavior in action

I agree option 3 would be the clearest, but I'm also not sure about how strict the formatting constraints are. Maybe the docs team could help make the choice.

I just saw the addition of the comment! Ignore my comment above.

That's interesting. I do like when an example is explicit, since this example includes 2 functions, then I suppose no reason why I can't either. I'll make that change for now, and request another review from both you and Kevin to see what you think.

Defer to Jeff's judgement since this example is requested by the documentation team.

src/libmongoc/tests/test-mongoc-sample-commands.c

kkloberdanz · 2022-11-14T21:36:54Z

src/libmongoc/tests/test-mongoc-sample-commands.c

+   /*
+    * Perform the following aggregation pipeline, and accumulate the count in
+    * `adoptable_pets_count`.
+    *
+    *  adoptablePetsCount = db.cats.aggregate(
+    *      [ { "$match": { "adoptable": true } },
+    *        { "$count": "adoptableCatsCount" } ], session=s
+    *  ).next()["adoptableCatsCount"]
+    *
+    *  adoptablePetsCount += db.dogs.aggregate(
+    *      [ { "$match": { "adoptable": True} },
+    *        { "$count": "adoptableDogsCount" } ], session=s
+    *  ).next()["adoptableDogsCount"]
+    *
+    * Remember in order to apply the client session to
+    * this operation, you must append the client session to the options passed
+    * to `mongoc_collection_aggregate, i.e.,
+    *
+    * mongoc_client_session_append (cs, &opts, &error);
+    * cursor = mongoc_collection_aggregate (
+    *    collection, MONGOC_QUERY_NONE, pipeline, &opts, NULL);
+    */
+   accumulate_adoptable_count (cs, cats_collection, &adoptable_pets_count);
+   accumulate_adoptable_count (cs, dogs_collection, &adoptable_pets_count);


Jeff has checked off on using this comment as a part of the documentation.

kkloberdanz · 2022-11-14T22:06:32Z

FYI, Gil brought up that another C driver example is lengthy and has multiple functions, so it seems that if I were to include multiple functions as a part of the example, that would fit with how things are currently shown. I pushed that in order for us to compare and decide which way we would prefer.

For reference, the example that Gil found: https://www.mongodb.com/docs/v6.0/core/transactions/

This reverts commit 1069a12.

This reverts commit 036e287.

galon1

I think the example looks good! I just have one question about using python in this example.

src/libmongoc/tests/test-mongoc-sample-commands.c

kevinAlbs

LGTM with small fixes.

src/libmongoc/tests/test-mongoc-sample-commands.c

kevinAlbs · 2022-11-15T23:09:57Z

src/libmongoc/tests/test-mongoc-sample-commands.c

+
+   accumulate_adoptable_count (cats_collection, &adoptable_pets_count);
+   accumulate_adoptable_count (dogs_collection, &adoptable_pets_count);
+


Defer to Jeff's judgement since this example is requested by the documentation team.

src/libmongoc/tests/test-mongoc-sample-commands.c

Co-authored-by: Kevin Albertson <[email protected]>

kkloberdanz added 19 commits November 9, 2022 13:12

Add test_example_59

21e7653

Test skeleton contains comment of Python example code for what this example should perform.

setup boilerplate

d038c92

check cursor for results

12635e6

populating cats

103bcda

successfully comparing document with control

cacada0

goto cleanup

4df0e3c

lookup adoptableCatsCount

af7d801

seeding dogs collection

d697998

a little DRYer

b73bfe3

use snapshot session

433376a

fmt

8edc763

is_equal flag no longer used

3fc0e8a

clang-format

8138aba

check for error first

352afa4

destroy client

2985bb3

Merge branch 'master' into CDRIVER-4295

5f513ac

warning for using switch, using if/else instead

14b86b0

move boiler plate seeding into pet_setup

31d615a

move closer to related function

57d9a89

kkloberdanz marked this pull request as ready for review November 11, 2022 14:29

kkloberdanz requested review from kevinAlbs and galon1 November 11, 2022 16:33

kevinAlbs reviewed Nov 14, 2022

View reviewed changes

galon1 approved these changes Nov 14, 2022

View reviewed changes

galon1 reviewed Nov 14, 2022

View reviewed changes

src/libmongoc/tests/test-mongoc-sample-commands.c Show resolved Hide resolved

kkloberdanz added 4 commits November 14, 2022 13:14

added suggestions from Kevin

4657565

lld is not portable for int64_t, use macro PRId64 instead.

ffb07d0

end example before assertion

5709929

comment on how the aggregation pipeline is being performed

3c62a01

kkloberdanz commented Nov 14, 2022

View reviewed changes

kkloberdanz added 2 commits November 14, 2022 15:38

missing punctuation

c0479ab

show all example code

036e287

kkloberdanz requested review from galon1 and kevinAlbs November 14, 2022 22:04

kkloberdanz added 3 commits November 15, 2022 08:22

redundant

1069a12

Revert "redundant"

a467c55

This reverts commit 1069a12.

Revert "show all example code"

22890b2

This reverts commit 036e287.

galon1 reviewed Nov 15, 2022

View reviewed changes

src/libmongoc/tests/test-mongoc-sample-commands.c Show resolved Hide resolved

kevinAlbs approved these changes Nov 15, 2022

View reviewed changes

kkloberdanz and others added 3 commits November 16, 2022 08:13

Update src/libmongoc/tests/test-mongoc-sample-commands.c

479e472

Co-authored-by: Kevin Albertson <[email protected]>

Update src/libmongoc/tests/test-mongoc-sample-commands.c

a5e5a5e

Co-authored-by: Kevin Albertson <[email protected]>

Update src/libmongoc/tests/test-mongoc-sample-commands.c

4b242db

Co-authored-by: Kevin Albertson <[email protected]>

kkloberdanz merged commit 42296db into mongodb:master Nov 16, 2022

kkloberdanz deleted the CDRIVER-4295 branch November 16, 2022 15:47


		accumulate_adoptable_count (cats_collection, &adoptable_pets_count);
		accumulate_adoptable_count (dogs_collection, &adoptable_pets_count);

Add test_example_59 #1143

Add test_example_59 #1143

Uh oh!

Conversation

kkloberdanz commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs commented Nov 14, 2022

Uh oh!

galon1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galon1 Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkloberdanz commented Nov 14, 2022

Uh oh!

galon1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinAlbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kkloberdanz commented Nov 9, 2022 •

edited

Loading

galon1 left a comment •

edited

Loading

galon1 Nov 14, 2022 •

edited

Loading