Skip to content

DOCSP-39689 Network Compression standardization #70

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

Conversation

jordan-smith721
Copy link
Collaborator

Pull Request Info

PR Reviewing Guidelines

JIRA - https://jira.mongodb.org/browse/DOCSP-39689
Staging - https://preview-mongodbjordansmith721.gatsbyjs.io/java-rs/DOCSP-39689-network-compression/connect-to-mongo/network-compression/

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
Collaborator

@norareidy norareidy left a comment

Choose a reason for hiding this comment

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

LGTM + a couple small things!

@jordan-smith721 jordan-smith721 requested review from a team and vbabanin and removed request for a team August 14, 2024 18:06
@jordan-smith721
Copy link
Collaborator Author

Pinging @vbabanin in case this PR request got lost in the mix

@@ -0,0 +1,19 @@
// start-specify-connection-string

import java.util.Arrays;
Copy link
Member

@vbabanin vbabanin Aug 26, 2024

Choose a reason for hiding this comment

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

We specify the inclusion of Arrays as an import here, but it is never used in this example tab. It seems to be related to the next tab, where the compressorList() method is used.

However, even if we move the import of Arrays to the next tab, why do we only include this import and omit the imports like MongoClient and MongoClientSettings?

The previous page, 'TLS/SSL', includes a statement before examples that lists these import statements:

import com.mongodb.reactivestreams.client.MongoClients;
import com.mongodb.reactivestreams.client.MongoClient;

Perhaps it is assumed that users have read the previous page, 'TLS/SSL' and are aware of these imports. However, I don't think we should make this assumption.

I noticed in another PR that we followed the approach of duplicating the same information to ensure users have everything they need within the section they’re reading, regardless of prior sections. This is highlighted in the discussion here: #48 (comment).

Therefore, it seems that we should add not only the Arrays import but also MongoClients and MongoClientSettings imports to ensure completeness and consistency.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this! That import was added by mistake (I think my editor automatically added it without me noticing for the following code snippet in that same file). For these pages that focus on a specific topic and don't provide "fully runnable code examples," we typically don't show the imports, as these code examples are intended to show only the relevant lines of code for the operation itself (See the pages nested under "Write Data to MongoDB" for more examples of this). The TLS/SSL page has not yet been standardized so it doesn't follow this convention yet.

Copy link
Member

Choose a reason for hiding this comment

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

Understood, thank you for clarification!

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for docs-java-rs ready!

Name Link
🔨 Latest commit 601b73a
🔍 Latest deploy log https://app.netlify.com/sites/docs-java-rs/deploys/66d204bc3b4df400088c01ab
😎 Deploy Preview https://deploy-preview-70--docs-java-rs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jordan-smith721 jordan-smith721 merged commit a4ce124 into mongodb:main Aug 30, 2024
4 of 5 checks passed
@jordan-smith721 jordan-smith721 deleted the DOCSP-39689-network-compression branch August 30, 2024 17:46
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