Skip to content

Conversation

@kkloberdanz
Copy link
Contributor

@kkloberdanz kkloberdanz commented Feb 22, 2023

CXX-2425

@kkloberdanz kkloberdanz marked this pull request as ready for review March 22, 2023 15:43
@kkloberdanz kkloberdanz requested review from eramongodb, kevinAlbs and vector-of-bool and removed request for vector-of-bool March 22, 2023 15:43
@kevinAlbs
Copy link
Collaborator

Commits in DRIVERS-2017 also include tests to validate the unified test runner implementation.
I suggest copying the following test files to data/unified-valid-pass from the last modified commit (mongodb/specifications@d8be9ce) which includes the test simplification of CXX-2538:

source/unified-test-format/tests/valid-pass/kmsProviders-explicit_kms_credentials.json
source/unified-test-format/tests/valid-pass/kmsProviders-placeholder_kms_credentials.json
source/unified-test-format/tests/valid-pass/kmsProviders-unconfigured_kms.json
source/unified-test-format/tests/valid-pass/kmsProviders-mixed_kms_credential_fields.json

Kyle Kloberdanz and others added 15 commits March 22, 2023 15:38
@kkloberdanz kkloberdanz requested a review from eramongodb March 30, 2023 19:28
Copy link
Contributor

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

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

Please replace the const std::string& parameters for get_key_by_alt_name and remove_key_alt_name with bsoncxx::string::view_or_value and revert the return type of these functions + the get_key and add_key_alt_name functions to stdx::optional<bsoncxx::document::value>. Consistency with public API was specific to the string-like parameters, not the return type. stdx::optional<bsoncxx::document::value> was the correct return type for these functions.

In addition to some minor suggestions to avoid redundant copies, otherwise LGTM.

@kkloberdanz kkloberdanz merged commit 677aa9d into mongodb:master Mar 31, 2023
@kkloberdanz kkloberdanz deleted the CXX-2425 branch March 31, 2023 17:15
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