Skip to content

Conversation

@norareidy
Copy link
Collaborator

@norareidy norareidy commented Dec 21, 2023

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-30557
Staging - https://preview-mongodbnorareidy.gatsbyjs.io/rust/DOCSP-30557-faq/faq/

Self-Review Checklist

  • Is this free of any warnings or errors in the RST?
  • Did you run a spell-check?
  • Did you run a grammar-check?
  • Are all the links working?
  • Are the facets and meta keywords accurate?

Copy link
Contributor

@ccho-mongodb ccho-mongodb left a comment

Choose a reason for hiding this comment

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

Plenty of helpful information here!

I left some comments mainly on making sure the information presented is clearly explained. Let me know if you have any questions.

@norareidy norareidy requested a review from ccho-mongodb January 3, 2024 21:20
Copy link
Contributor

@ccho-mongodb ccho-mongodb left a comment

Choose a reason for hiding this comment

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

Added a few more suggestions and issues, but otherwise LGTM. If you would like me to take another look after making updates, let me know!

source/faq.txt Outdated
Comment on lines 57 to 60
conversions between custom Rust types and BSON. You can install the ``serde``
crate to access the functionality of the Serde framework. For instructions
on installing this crate, see `serde <https://crates.io/crates/serde>`__ in the
crates registry.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think it would be helpful to stay consistent with the terminology (whether "add the crate" or "install"). Based on
the prior comment and the first sentence of the next paragraph, perhaps this should be "add the crate".

source/faq.txt Outdated

After you add the crate to your application, you can model the documents in a collection
by using a custom type instead of a BSON document. The following example includes a ``derive``
statement before the ``Vegetable`` struct definition, which instructs the driver to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:
From the Derive page in Rust by Example, it seems like "derive" should be an "attribute" rather than "statement".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point - will fix in other Rust docs too

source/faq.txt Outdated
Comment on lines 182 to 183
- ``Ok(T)``, which wraps the value of the result of the operation
- ``Err(E)``, which wraps an error value if the operation is unsuccessful
Copy link
Contributor

Choose a reason for hiding this comment

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

Issue:

The style guide on List Items provides specific guidance on list items that separate the initial term and information about it:

If you need to separate the initial term or phrase from the information that follows it, use a colon. However, if you don't need a separator, don't use one. (For an example of a list in which separators aren't necessary, see the list at the top of this topic.

Suggestion:

Suggested change
- ``Ok(T)``, which wraps the value of the result of the operation
- ``Err(E)``, which wraps an error value if the operation is unsuccessful
- ``Ok(T)``: wraps the value of the result of the operation if the operation is successful
- ``Err(E)`: wraps an error value if the operation is unsuccessful

Applies to all similar instances.

source/faq.txt Outdated
Comment on lines 193 to 194
To access the result of ``insert_one()``, you can use the ``?`` operator to unwrap its
return value. If the operation is successful, the ``?`` operator unwraps the ``InsertOneResult``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think the first part of the sentence should directly introduce the rest of the sentence

In the current sentence, the first part mentions accessing the result and the rest of it explains only how to unwrap the return value.

Suggested change
To access the result of ``insert_one()``, you can use the ``?`` operator to unwrap its
return value. If the operation is successful, the ``?`` operator unwraps the ``InsertOneResult``
To access the unwrapped result of ``insert_one()``, use the ``?`` operator. If the operation is successful, the ``?`` operator unwraps the ``InsertOneResult``

source/faq.txt Outdated
Comment on lines 204 to 206
Alternatively, you can create custom logic for handling the ``InsertOneResult`` value
depending on whether ``insert_one()`` returns the value wrapped in the ``Ok`` or the ``Err``
variant. The following code uses the ``match`` keyword to process the ``insert_one()``
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

No change necessary, but I think this might be a more succinct way to present this sentence since Result (InsertOneResult, once that equivalence is added) was explained earlier:

Suggested change
Alternatively, you can create custom logic for handling the ``InsertOneResult`` value
depending on whether ``insert_one()`` returns the value wrapped in the ``Ok`` or the ``Err``
variant. The following code uses the ``match`` keyword to process the ``insert_one()``
Alternatively, you can create a conditional to handle the unwrapped values of ``InsertOneResult``. The following code uses the ``match`` keyword to process the ``insert_one()``

source/faq.txt Outdated
- `Result <https://doc.rust-lang.org/std/result/enum.Result.html>`__
- `Option <https://doc.rust-lang.org/std/option/enum.Option.html>`__
- `? Operator <https://doc.rust-lang.org/rust-by-example/std/result/question_mark.html>`__
- `If Let <https://doc.rust-lang.org/book/ch06-03-if-let.html>`__ No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think this could be confusing since the keywords "if" and "let" are lowercase in their usage and the content of the link describes behavior of using two keywords in a pattern rather than adjacent to each other:

Suggested change
- `If Let <https://doc.rust-lang.org/book/ch06-03-if-let.html>`__
- Control Flow with `if let <https://doc.rust-lang.org/book/ch06-03-if-let.html>`__

source/faq.txt Outdated
The ``Option`` enum can return the following variants:

- ``None``, which represents an empty value returned by an operation
- ``Some(T)``, which wraps a nonempty return value
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think "nonempty" is usually spelled with a hyphen ("non-empty").

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think the placement of the Result example directly after this makes it awkward. Instead, perhaps placing this before the line that starts with "Some {+driver-short+} methods return an Option type, such as the read_concern()..." would make the information more organized and easier to locate.

source/faq.txt Outdated
}

You can then create a ``Collection`` instance with your custom struct type as its
generic type parameter. The following example instantiates the ``my_coll`` collection
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

I think lowercase collection might imply a collection name rather than the Collection struct.

Suggested change
generic type parameter. The following example instantiates the ``my_coll`` collection
generic type parameter. The following example instantiates the ``my_coll`` Collection

Alternatively, perhaps "variable" would be more conventional since this describes assignment rather than direct instantiation.

@norareidy norareidy merged commit bd90347 into mongodb:master Jan 9, 2024
@norareidy norareidy deleted the DOCSP-30557-faq branch January 9, 2024 18:54
norareidy added a commit that referenced this pull request Jan 9, 2024
(cherry picked from commit bd90347)
norareidy added a commit that referenced this pull request Jan 9, 2024
(cherry picked from commit bd90347)
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