Code Standardization and Cleanup

[JerrettDavis]

This is dependent on #38, or rather, it includes the changes from #38.

I took a look over the code and tried to standardize and correct a few inconsistencies. I also added a couple TODOs to help draw attention to additional areas that might lead to confusion, but where the changes would potentially be breaking. I primarily addressed the following things:

• Code style - File-Scoped Namespaces
• Code style - Line length
  • Tried to get most lines down to around 80 characters to make code more readable. Makes it possible to view multiple code windows side-by-side without any horizontal scrolling.
• Code style - Reference type nullability
  • Updated nullability and references wherever possible based on usage and API definitions.
• Async/Await Usage
  • There are many extraneous async/await usages. I audited the calls and updated functions to directly return Task/Task<T> where appropriate. This change lets callers handle awaiting and will simplify future error handling.
• Documentation
  • Updated XML documentation where appropriate
• Reduced code duplication
  • Several of the API models were declared multiple times. I updated several models to reuse existing models rather than reimplementing the details/options classes.
    • ShowModelResponse.Details had its type changed to Details. ShowModelResponseDetails removed
    • RunningModel changed to inherit from Model. Duplicate properties removed.
    • GenerateEmbeddingRequest.Options had its type changed to RequestOptions from string.

[awaescher]

Highly appreciate the work you have done here, but I won't merge the code with TODO comments for now.

I was aware of the inconsistent file format as the first pull requests came in - mostly whitespacing (tabs/spaces) and I planned to add an editorconfig to address this if there's time. But I didn't want to reject contributions just because of their coding style. Thanks a lot for helping me out with this.

One thing about this pull request though:

I am not a fan of sub 80-char line limits as imo they add a lot of noise. This is very subjective of course but for me this particularly complex code ...

public async Task<IEnumerable<Message>> SendAs(ChatRole role, string message, IEnumerable<string> imagesAsBase64, CancellationToken cancellationToken = default)
{
  _messages.Add(new Message(role, message, imagesAsBase64?.ToArray()));

  var request = new ChatRequest
  {
    Messages = Messages.ToList(),
    Model = Client.SelectedModel,
    Stream = true
  };

  var answer = await Client.SendChat(request, Streamer, cancellationToken);
  _messages = answer.ToList();
  return Messages;
}

... is way easier to read than this:

public async Task<IEnumerable<Message>> SendAs(
  ChatRole role,
  string message,
  IEnumerable<string>? imagesAsBase64,
  CancellationToken cancellationToken = default)
{
  _messages.Add(new Message(
    role, message, imagesAsBase64?.ToArray()));

  var request = new ChatRequest
  {
    Messages = Messages.ToList(),
    Model = Model,
    Stream = true
  };

  var answer = await Client.SendChat(
    request, Streamer, cancellationToken);
  _messages = answer.ToList();
  return Messages;
}

Mostly because the lines are more "atomic" and the code structure is easier to capture visually. It also emphasizes the argument list less, which is also not that important as the code itself.

I do break some lines by myself if I think it makes sense but certainly not for a virtual character limit. This idea is from the 80s where people didn't have phones with 4k displays. This rule is highly obsolete to me.

That being said, this is not a show stopper for me. But I might go through a few code parts after merging and reformat them again, just that you know.

[JerrettDavis]

Thank you for the input @awaescher.

• Agreed on the editorconfig inclusion. I just wanted to try to get things mostly right based on the prevailing code styles I saw present. Always more important to get that initial logic implemented before fussing too much about style.
• In the areas with TODOs, I did not rename the models yet. I noted them with TODOs because renaming the type could be a breaking change for some and could require a semver minor version increment. Do we want to go ahead and rename the classes/models in this branch?
• Totally agree on the arbitrariness of the 80 character line limit. I tend to target it as a goal since there are quite a few languages and teams that use it or 120 characters as a limit. I find if I target 80 characters as a soft limit and 120 characters as a hard limit, the code is fully scannable with no horizontal scrolling required and it's almost guaranteed to be presentable on any screen.
  • For example, here is a screenshot of your code and my code presented directly here on this GitHub pull request. Notice how your code cannot be read in its entirety without scrolling horizontally within the code window to see the final parameter.
    
  • Additionally, in an editor, the 80/120 limit allows for multiple editor windows to be placed side-by-side with absolutely no horizontal scrolling required. Code tends to read as a pretty flat, vertical set of instructions that you can read line-by-line to quickly contextualize.
    
  • Feel free to change the line lengths as you see fit! I just wanted to contextualize the change from my side a bit.

[awaescher]

I am thanking you for this update and not telling me to gtfo after beefing over the free work you wanted to share 😅

• In meantime I prepared an editorconfig file and tried a dotnet format but I am not doing this now as this would make your pull request impossible to merge, I guess. I would love to give this a chance first.

• Also I did forget to refer to your original line here:

  There are many extraneous async/await usages. I audited the calls and updated functions to directly return Task/Task where appropriate. This change lets callers handle awaiting and will simplify future error handling.

  I am doing this on purpose. Returning Tasks directly might be measurable in nanoseconds but has a major disadvantage and that is that I found a few times where (experienced) people added try-catch-blocks to these methods. But they never catched exceptions as the tasks were just returned and not executed within these blocks. I can live with both variants, but this was on purpose and I think it's the safer way to go.

• I see your samples and the triple-diff and I understand. Again, for me this is not an issue because as I wrote

  It also emphasizes the argument list less ...

  With that I mean I mostly don't care too much for longer argument lists. In the screenshot where my code is capped, that's perfectly fine for me because in most cases, the arguments won't change. And if they do, it's super easy to scroll horizontially for a second. It's not that I don't mind the text being out of range, I actually prefer it that way. This is not true for "real" code, that means instructions. And that's when I break lines, but mostly before the dots . to make it well structured (imo), like this:

  return Context
    .Get<User>()
    .Where(u => u.IsActive)
    .Select(u => u.Email)
    .ToList();

  (notice how this makes every line "atomic" again)

---

These are all personal preferences, I don't want to convince anyone that my opinion is the best.

Would you do me the favor and remove the TODOs or introduce a breaking change? I could bump the major version for this.

[JerrettDavis]

• For the async side, when leveraging the library, I was getting a lot of HTTP-client specific issues (such as 404 when a given model wasn't found). My intention is to correct all the underlying async calls in a future PR to introduce library-specific errors that better illustrate what has failed. I also intended to move over to ConfigureAwait(false) in the future PR to help manage the context-switching and prevent deadlocks.

• Thanks for the further clarification on the code styles. I think we're in near complete agreement. I took a heavy hand in making everything standard. Please revise however you prefer.

• I will go ahead and make those breaking changes now. They'll be pushed shortly.

[awaescher]

Release is out → 2.0.1
Once again, thanks a lot for your help!

[HavenDV]

Hi guys.
I appreciate your efforts to support this, but would like to suggest a few steps forward to join forces.
I'm a fan of SDKs like this being generated from the OpenAPI specification. Although there is still no official one here, it may appear (ollama/ollama#3383), and there is a supported specification in the Dark language https://github.com/davidmigloz/langchain_dart/blob/main/packages/ollama_dart/oas/ollama-curated.yaml. The advantage is that we focus on supporting the specification across all languages, rather than the implementation features of a specific language. At the same time, we can still provide additional extensions for the generated code to ensure maximum convenience for users.
It also gives facilities like Trimming/NativeAOT/nullable enable/Enums/AnyOf/OneOf out of the box

This is what the generated code looks like:

• https://github.com/tryAGI/Ollama/tree/main/src/libs/Ollama/Generated/OpenApiGenerator/OpenApiGenerator.SdkGenerator
• https://github.com/tryAGI/Ollama/tree/main/src/libs/Ollama.Models/Generated/OpenApiGenerator/OpenApiGenerator.SdkGenerator

It is generated from this specification:
https://github.com/tryAGI/Ollama/blob/main/docs/openapi.yaml

It's not 100% perfect and feedback is still needed, but it's a solution that will work without the need to manually maintain specific implementation details

[JerrettDavis]

Hi @HavenDV, I'm a big fan of leveraging OpenAPI clients when they're available! I also agree that it'd be great to have some deduplication of efforts. Can you open an issue to track the effort?

With no OpenAPI spec currently, officially released, it's a bit of a moving target. But I think it would be a worthwhile endeavor to have an exploratory branch to explore how that might look.

[awaescher]

+1 for the issue, or even better a discussion which I just enabled for this repository.