Skip to content
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.

Sign up for GitHub

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

Jump to bottom

DRIVERS-2789 Use Markdown for Specifications Documentation #1482

Merged
merged 22 commits into from
Jan 4, 2024
Merged

DRIVERS-2789 Use Markdown for Specifications Documentation #1482

merged 22 commits into from
Jan 4, 2024

Conversation

blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 7, 2023 •
edited
Loading

Please complete the following before merging:

  • Update changelog.
  • Make sure there are generated JSON files from the YAML test files.
  • Test changes in at least one language driver.
  • Test these changes against all server versions and topologies (including standalone, replica set, sharded clusters, and serverless).

@blink1073 blink1073 requested review from a team as code owners December 7, 2023 15:47
@blink1073 blink1073 requested review from dariakp and jmikola and removed request for a team and dariakp December 7, 2023 15:47
@blink1073 blink1073 requested review from a team as code owners December 7, 2023 18:26
@blink1073 blink1073 requested review from JamesKovacs, durran, vector-of-bool and jess-sig and removed request for a team December 7, 2023 18:26
@blink1073 blink1073 marked this pull request as ready for review December 8, 2023 13:31
jmikola
jmikola reviewed Dec 11, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
source/client-side-operations-timeout/client-side-operations-timeout.rst Show resolved Hide resolved
source/enumerate-collections.rst Outdated Show resolved Hide resolved
source/read-write-concern/read-write-concern.rst Outdated Show resolved Hide resolved
source/transactions/transactions.rst Outdated Show resolved Hide resolved
source/uri-options/uri-options.rst Outdated Show resolved Hide resolved
source/index-management/index-management.rst Outdated Show resolved Hide resolved
source/crud/crud.rst Outdated Show resolved Hide resolved
source/client-side-operations-timeout/client-side-operations-timeout.md Outdated
amount of time that a single operation can execute before control is
returned to the user. This timeout applies to all of the work done to
execute the operation, including but not limited to server selection,
connection checkout, and server-side execution.
Copy link
Member

Choose a reason for hiding this comment

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

Did you plan on enforcing the line length with linting tools? Knowing how fast and loose folks are when editing specs (I've seen entire paragraphs written without line breaks), I think it'd be a matter of time before the line lengths get inconsistent.

Also, any thoughts on 120 vs. 80? When link syntax is involved, the 80-column limit results in some very aggressive wrapping.

Copy link
Member Author

Choose a reason for hiding this comment

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

Line length is not enforceable with mdformat.

jmikola
jmikola reviewed Dec 11, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
blink1073 and others added 9 commits December 12, 2023 06:29
@blink1073 @jmikola
Update source/enumerate-collections.rst
6d89610 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/read-write-concern/read-write-concern.rst
0222bbe 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/transactions/transactions.rst
cd9c780 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/uri-options/uri-options.rst
e90856f 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/index-management/index-management.rst
2d831a2 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/crud/crud.rst
cdc5d02 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update README.md
a67bf37 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073
address review
c767ca6
@blink1073
fix link
1d074a3
jmikola
jmikola approved these changes Dec 18, 2023
View reviewed changes
Copy link
Member

@jmikola jmikola left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A few more comments about link anchors with the wrong character case.

And I'm still in favor of 120 instead of 80 for character line limits on spec docs; the last comment in #1482 (comment) didn't acknowledge my question about that.

Happy to defer review to the actual CSOT spec owner(s) to sort the rest of this out.

source/change-streams/change-streams.rst Outdated Show resolved Hide resolved
source/uri-options/uri-options.rst Outdated Show resolved Hide resolved
source/uri-options/uri-options.rst Outdated Show resolved Hide resolved
source/uri-options/uri-options.rst Outdated Show resolved Hide resolved
blink1073 and others added 5 commits December 18, 2023 09:34
@blink1073 @jmikola
Update source/change-streams/change-streams.rst
ec15ebc 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/uri-options/uri-options.rst
c9f0e7e 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/uri-options/uri-options.rst
0faa3f3 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073 @jmikola
Update source/uri-options/uri-options.rst
fe25ba3 
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
@blink1073
Wrap at 120 chars
4058eed
@blink1073
Copy link
Member Author

And I'm still in favor of 120 instead of 80 for character line limits on spec docs

Fixed.

ShaneHarvey
ShaneHarvey approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@ShaneHarvey ShaneHarvey left a comment

Choose a reason for hiding this comment

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

LGTM

@blink1073 blink1073 merged commit eef59d9 into mongodb:master Jan 4, 2024
4 checks passed
@blink1073 blink1073 deleted the DRIVERS-2789 branch January 4, 2024 21:58
@blink1073 blink1073 mentioned this pull request Feb 9, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShaneHarvey ShaneHarvey ShaneHarvey approved these changes

@jmikola jmikola jmikola approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blink1073 @jmikola @ShaneHarvey