-
Notifications
You must be signed in to change notification settings - Fork 89
(DOCSP-39523): Consolidate Call a Function page #3322
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
(DOCSP-39523): Consolidate Call a Function page #3322
Conversation
Readability for Commit Hash: d0fab64 You can see any previous Readability scores (if they exist) by looking Readability scores for changed documents:
For Grade Level, aim for 8 or below. For Reading Ease scores, aim for 60 or above:
For help improving readability, try Hemingway App. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again, @dacharyc! Mostly looks good, but I'm requesting changes for a few reasons:
- Some Java API doc links weren't working as expected
- Swift API details have headings that don't appear in other language's details content
- A suggestion for potentially removing the Atlas Function example
source/sdk/atlas/call-function.txt
Outdated
The example on this page demonstrates calling an :ref:`Atlas Function <functions>` | ||
named ``sum`` or ``concatenate`` that takes two arguments, sums or concatenates | ||
them, and returns the result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph is confusing. Here are a few thoughts:
In the following code block, we're not showing calling the Function from the SDK - we're showing the Atlas Function itself. The current set up is a bit misleading. I think "The example on this page" may not mean the closest code example, but that's how most people will read it.Why would it matter what the Atlas Function is named? We don't show that in the following code block.While it's true that the code example could sum or concatenate in JavaScript, this wouldn't work in other languages. I think it would make sense to simplify to just sum or concatenate and not worry about JavaScript's weirdness
EDIT: I see what you were going for. See next comment for more well-informed thoughts. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I think the distance between stating what the Atlas Function is and the example(s) where the Function is invoked is too much for readers to easily associate the two.
- Did you consider putting the Atlas Function example in the "Call a Function" section - maybe it's own subsection - where it's more clearly highlighted that this is the Function that the following code examples will invoke?
- I'm tempted to remove the Atlas Function example entirely. In the context of the consolidated examples, it's more confusing than helpful, I think. It would be nice to have, but I think there are some fundamental things we'd need to fix that are beyond the scope of this PR. (like the fact that the Function example explicitly states it's concatenating strings in a comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did experiment with a few different placements for it, but landed on this. But I understand it's not ideal.
I've removed the example and have reworded the description of the Function we're calling to be generic enough to apply to all the examples, I think.
We can revisit adding some other example in the future if this page becomes a high enough priority to warrant a bigger time investment.
In this case, ``functions.concatenate()`` refers to the ``concatenate`` | ||
function. Pass a ``BSONArray`` of arguments. | ||
|
||
Async/Await |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Async/Await and Completion Handler headings are included in the on-this-page TOC for every language. But they only work for Swift because it's the only language with API details that include these headings.
Is the only way around this to remove the headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, yes, I meant to make this page depth 1. Good catch, thank you!
source/includes/api-details/java/atlas/call-atlas-function-call-function-description.rst
Outdated
Show resolved
Hide resolved
source/includes/api-details/java/atlas/call-atlas-function-call-function-description.rst
Outdated
Show resolved
Hide resolved
source/includes/api-details/java/atlas/call-atlas-function-call-function-description.rst
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,18 @@ | |||
To execute a function in Swift, use the ``functions`` object on the currently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that the Swift content doesn't link to the Swift API ref docs. Worth adding links?
- id: typescript | ||
content: | | ||
|
||
.. literalinclude:: /examples/MissingPlaceholders/example.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can share the JS example with TS here, as there wouldn't be any difference between the two.
Co-authored-by: Kyle Rollins <115574589+krollins-mdb@users.noreply.github.com>
✅ Deploy Preview for device-sdk ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks again, Dachary!
e7fa389
into
mongodb:feature-consolidated-sdk-docs
✨ Staging URL: https://preview-mongodbmongodb.gatsbyjs.io/realm/feature-consolidated-sdk-docs/ 🪵 Logs |
Pull Request Info - SDK Docs Consolidation
Jira ticket: https://jira.mongodb.org/browse/DOCSP-39523
Staged Page
Page Source
Add links to every SDK's pages where you got the SDK-specific information:
PR Author Checklist
Before requesting a review for your PR, please check these items:
feature-consolidated-sdk-docs
branch instead ofmaster
Naming
.rst
files comply with the naming guidelinesLinks and Refs
Content
Reviewer Checklist
As a reviewer, please check these items: