Skip to content

Conversation

mcmorisi
Copy link
Collaborator

Adapted from the C++ draft and Kotlin Coroutine versions of this page.

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-41152
Staging - https://preview-mongodbmcmorisi.gatsbyjs.io/kotlin-sync/DOCSP-41152-time-series/data-formats/time-series/

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

netlify bot commented Jul 29, 2024

👷 Deploy request for docs-kotlin-sync pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 6d2e1f5

Copy link
Collaborator

@shuangela shuangela left a comment

Choose a reason for hiding this comment

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

good job! a few quick fixes


- The name of the new collection to create

- A `CreateCollectionOptions <{+core-api+}/com/mongodb/client/model/CreateCollectionOptions.html>`__
Copy link
Collaborator

Choose a reason for hiding this comment

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

S: The API docs state that CreateCollectionOptions is a class, change "object" to "class"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In Java, an object refers to an individual instantiation of a class (i.e. a class is the "template" and the object is the thing made from the template). In this case, I think it makes sense to leave this as "object".

- The name of the new collection to create

- A `CreateCollectionOptions <{+core-api+}/com/mongodb/client/model/CreateCollectionOptions.html>`__
object containing the `TimeSeriesOptions <{+core-api+}/com/mongodb/client/model/TimeSeriesOptions.html>`__ object
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: Containing seems like a weird word choice here because in the example, timesseriesoptions is chained to createcollectionoptions. If it was containing, i'd expect TimeSeriesOptions to be a parameter.

S: Clarify that you chain TimeSeriesOptions to CreateConnectionOptions to create a timeseries collection

@mcmorisi mcmorisi requested a review from shuangela July 29, 2024 19:29
Copy link
Collaborator

@shuangela shuangela left a comment

Choose a reason for hiding this comment

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

lgtm

@mcmorisi mcmorisi requested review from a team and rozza and removed request for a team July 30, 2024 13:09
Copy link
Member

@rozza rozza left a comment

Choose a reason for hiding this comment

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

One optional nit but either way LGTM!

val results = database.listCollections()

results.forEach { result ->
println(result.toJson(JsonWriterSettings.builder().indent(true).build()))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: You could create the settings just once and reuse

@mcmorisi mcmorisi merged commit c8d9955 into mongodb:master Jul 31, 2024
1 of 2 checks passed
@mcmorisi mcmorisi deleted the DOCSP-41152-time-series branch July 31, 2024 13:13
mcmorisi added a commit that referenced this pull request Jul 31, 2024
(cherry picked from commit c8d9955)
mcmorisi added a commit that referenced this pull request Jul 31, 2024
(cherry picked from commit c8d9955)
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