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

Source code cleanups #163

Merged
merged 5 commits into from
Jan 8, 2025
Merged

Conversation

Joy-less
Copy link
Contributor

@Joy-less Joy-less commented Jan 6, 2025

This pull request cleans up some of the source code:

  • Removed an unused class
  • Removed unused usings
  • Replaced full types with usings
  • Changed confusing syntax
  • Improved some comments
  • Removed unnecessary null coalescing operators

@@ -20,7 +21,7 @@ public void Append(ChatResponseStream? item)
{
_messageBuilder.Append(item);

if (item?.Done ?? false)
if (item is not null && item.Done)
Copy link
Owner

Choose a reason for hiding this comment

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

this is very subjective, I know, but I actually prefer item?.Done ?? false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood, this can be reversed.

src/MicrosoftAi/ChatOptionsExtensions.cs Show resolved Hide resolved
src/OllamaApiClientExtensions.cs Show resolved Hide resolved
@@ -5,7 +5,7 @@

namespace Tests;

#pragma warning disable CS8424 // The EnumeratorCancellationAttribute will have no effect. The attribute is only effective on a parameter of type CancellationToken in an async-iterator method returning IAsyncEnumerable
#pragma warning disable CS8424
Copy link
Owner

Choose a reason for hiding this comment

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

why would you remove the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already tells you this message if you hover over CS8424 in VSCode or Visual Studio, but it can be reversed if you want to keep it.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, you'd need tooling support for that. Might not be there on GitHub on the web, for example. I'd prefer to keep it

Copy link
Owner

Choose a reason for hiding this comment

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

why would you remove those? it's an extension method that can be used in other projects

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My mistake, I thought it was internal like StreamingChatCompletionUpdateAppender.cs and StreamingChatCompletionUpdateBuilder.cs which can be safely removed.

@Joy-less
Copy link
Contributor Author

Joy-less commented Jan 8, 2025

@awaescher I've made changes based on your feedback, please take a look.

@awaescher awaescher merged commit 44aa1b4 into awaescher:main Jan 8, 2025
1 check passed
@Joy-less Joy-less deleted the source-code-cleanups branch January 8, 2025 22: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.

2 participants