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

Added Documentation, Updated Access Modifiers, and Corrected Null Checking/Assertions #142

Merged
merged 15 commits into from
Nov 25, 2024

Conversation

JerrettDavis
Copy link
Contributor

@JerrettDavis JerrettDavis commented Nov 13, 2024

As mentioned in #113, it would be nice to have some sort of auto-generated documentation site for the library's API.

I updated some of the in-code documentation and created a Demo Site.

While reviewing the generated site, I altered the visibility of several classes to be internal. They appeared to largely be internal constructs only used by the OllamaSharp library itself.


Document Site

Demo Site Home Page:
image

Demo Site - Custom Docs
imageimage

Demo Site - Auto Generated API Docs
image
image


The documentation is being generated using docfx. It is highly configurable, and should meet a wide variety of use cases. I've done around the minimum amount of configuration necessary to get it up and running. I did add a plugin to make handing layouts a bit easier, and with that, I also added some CSS styles to help create more sophisticated layouts.

I wasn't sure what to use for the icon, and I didn't want to create confusion by using the official Ollama logo throughout. I created a simple, silly .NETish themed Ollama logo derivative. Please feel free to replace this with whatever appropriate branding you deem fit. I wanted to make sure it wasn't the default.


Class Access Modifiers

To minimize the scope of the documentation, I went through and adjusted several classes to have different access modifiers. I reduced their scope from public down to internal. The modified classes should be audited, and if they shouldn't be internal, it would be useful to add documentation around how one could extend those pieces of the library.


Documentation

I updated documentation throughout the library. In most of the places, the changes were limited to: adding documentation for return types, adding <see ref=""> and <see href=""> to relevant documentation, adding example code snippets, and clarifying verbiage.

Nullable Annotations

I noted several places where null-checking, null-asserting, or null reference type usage was inconsistent. In these locations, I changed the null-annotations by adding or removing a ! or ? to the affected code. In at least one area, I added a more explicit exception where a null could be passed into an area where one is not permitted.

@awaescher
Copy link
Owner

JD what the ... 😵
image

@JerrettDavis
Copy link
Contributor Author

@awaescher Whoops! I was tinkering a bit with the website build. Reverted the static includes.

The github action should handle all of this in its own branch without polluting anything else.

@awaescher
Copy link
Owner

Feels way better now. Wow, this seems to be really cool - I'll let you tinker further as long as it is a draft, okay?

@JerrettDavis
Copy link
Contributor Author

@awaescher I believe I've gotten the documentation into a workable state. The demo site has been updated.

@awaescher awaescher marked this pull request as ready for review November 18, 2024 06:38
@awaescher
Copy link
Owner

awaescher commented Nov 18, 2024

Outstanding work, thank you so much for the work you put into this!
(I just set this thing from draft to open - feel free to change the state as you want)

@JerrettDavis
Copy link
Contributor Author

All good with me! I definitely need a second set of eyes to review and make sure I didn't make something internal or shift a type-specification where we expect to have public API coverage.

@JerrettDavis
Copy link
Contributor Author

@awaescher I think it's all good from my end now. Please let me know if you have any questions or change requests. I do want to expand the documentation further, but there's probably more than enough in this PR already.

@awaescher
Copy link
Owner

I am on the road today, I will have another look at it tomorrow or the day after tomorrow.

docs/getting-started.md Outdated Show resolved Hide resolved
docs/getting-started.md Outdated Show resolved Hide resolved
src/Chat.cs Show resolved Hide resolved
src/OllamaSharp.csproj Show resolved Hide resolved
OllamaSharp.FunctionalTests/OllamaApiClientTests.cs Outdated Show resolved Hide resolved
src/OllamaSharp.csproj Show resolved Hide resolved
src/Chat.cs Show resolved Hide resolved
@awaescher awaescher changed the base branch from main to docs November 25, 2024 16:00
@awaescher awaescher merged commit 771afda into awaescher:docs Nov 25, 2024
1 check passed
@awaescher
Copy link
Owner

I merged it to a local branch, so that I can easiy check it out locally.
But most important: Once again a HUGE THANK YOU for the efforts you put into this. I really really appreciate it!

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.

2 participants