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.

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

CSHARP-4783: API docs generating with docfx #1206

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

sanych-sun
Copy link
Member

@sanych-sun sanych-sun commented Oct 19, 2023

@sanych-sun sanych-sun requested a review from a team as a code owner October 19, 2023 16:06
@sanych-sun sanych-sun requested review from adelinowona and JamesKovacs and removed request for a team and adelinowona October 19, 2023 16:06
Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

This looks great. Minor feedback on naming. As well since this commit replaces Sandcastle with DocFX, we should remove the ./Docs folder as part of this commit. I realize that this will remove both the Sandcastle-related files as well as the reference docs. The reference docs aren't related, but we stopped updating them awhile ago.

.gitignore Outdated
@@ -68,3 +68,5 @@ artifacts
packages
/tools
/evergreen/krb5.conf.empty
/build
Copy link
Contributor

Choose a reason for hiding this comment

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

Other tasks build into the artifacts directory. We should use that rather than adding another directory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ups, I've discovered that test results file being output into the ./build directory and did not check if other tasks output to the same folder. Will update here and some another tasks (packages creation script also output to the ./build folder now, will update it to use ./artifacts folder).

@@ -0,0 +1 @@
# MongoDB C# Driver API Reference
Copy link
Contributor

Choose a reason for hiding this comment

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

MongoDB .NET/C# Driver API Reference

@@ -0,0 +1,12 @@
#!/usr/bin/env bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of our other scripts use verb-noun.sh. Consider changing this to build-apidocs.sh.

Copy link
Member Author

@sanych-sun sanych-sun Oct 21, 2023

Choose a reason for hiding this comment

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

I probably would like to review this naming convention. If we use opposite convention: noun-verb.sh this let us group together related scripts. For example:

apidocs-build
apidocs-validate
packages-pack
packages-push
packages-test
tests-run

Let's discuss it with the team.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've reviewed other drivers/repos: all of them are using verb-noun.sh notation. I'll update scripts to follow the same naming conventions. Thanks.

@@ -1305,7 +1305,7 @@ tasks:

- name: atlas-search-index-helpers-test
commands:
- func: run-atlas-search-index-helpers-test
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated change even if it is fixing whitespace.

@@ -0,0 +1,2 @@
.manifest
Copy link
Member Author

Choose a reason for hiding this comment

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

As we removed "old" Docs folder, should we rename this folder to Docs instead of docfx_project?

Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM

@@ -47,7 +47,7 @@ functions:
fi
Copy link
Member Author

Choose a reason for hiding this comment

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

I've applied proper naming to all recently added scripts/tasks. Also I'll create a separate green-build-friday task to implement CI step to call docfx.

Copy link
Contributor

@JamesKovacs JamesKovacs left a comment

Choose a reason for hiding this comment

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

Cake includes a DocFX add-in. We should keep the ApiDocs task and use that add-in. If we decide to replace Cake entirely with shell scripts eventually, we can do it then. But for now we should keep the Cake tasks for consistency.

@sanych-sun sanych-sun force-pushed the doc_fx branch 4 times, most recently from 2fcd099 to 10d5c9c Compare December 7, 2023 23:49
@sanych-sun sanych-sun merged commit 588b29f into mongodb:master Dec 8, 2023
@sanych-sun sanych-sun deleted the doc_fx branch December 8, 2023 01:43
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.

None yet

2 participants