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

Fix:SemanticKernelQuickStart for c# #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chitangchin
Copy link

Fixing Microsofts Semantic Kernel Lights Demo

1. Adding Required attribute to LightModel Class Property Name

Issue:

Non-nullable property 'Name' must contain a non-null value when exiting constructor. Consider adding the 'required' modifier or declaring the property as nullable.CS8618

Proposal

LightsPlugin.cs - Line: 46

Original:

[JsonPropertyName("name")]
public string Name { get; set; }

Updated:

[JsonPropertyName("name")]
public required string Name { get; set; }

Why:

String property name must not allow null

2. Providing Null forgiving operator to UserInput

Issue:

Possible null reference argument for parameter 'content' in 'void ChatHistory.AddUserMessage(string content)'.CS8604

Proposal

Program.cs - Line: 43

Original:

    history.AddUserMessage(userInput);

Updated:

    history.AddUserMessage(userInput!);

Why:

While loop condition prevents userInput from being null, therefore we can provide a null forgiving operator at line 43.

3. Unecessary Async functions

Issue:

This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread.CS1998

Proposal

Program.cs - Line: 17 - 38

Original:

   [KernelFunction("get_lights")]
   [Description("Gets a list of lights and their current state")]
   public async Task<List<LightModel>> GetLightsAsync()
   {
      return lights;
   }

   [KernelFunction("change_state")]
   [Description("Changes the state of the light")]
   public async Task<LightModel?> ChangeStateAsync(int id, bool isOn)
   {
      var light = lights.FirstOrDefault(light => light.Id == id);

      if (light == null)
      {
         return null;
      }

      // Update the light with the new state
      light.IsOn = isOn;

      return light;
   }

Updated:

    [KernelFunction("get_lights")]
    [Description("Gets a list of lights and their current state")]
    public async Task<List<LightModel>> GetLightsAsync()
    {
        await Task.CompletedTask;
        return lights;
    }

    [KernelFunction("change_state")]
    [Description("Changes the state of the light")]
    public async Task<LightModel?> ChangeStateAsync(int id, bool isOn)
    {
        await Task.CompletedTask; // Dummy await

        var light = lights.FirstOrDefault(light => light.Id == id);

        if (light == null)
        {
            return null;
        }

        light.IsOn = isOn;
        return light;
    }

Why:

We should await for task completion within our async function

4. Simplifying collection initialization - .NET clean code standards

Collection initialization can be simplified IDE0028

Proposal

(LightsPluginFileName).cs - Line: 8 - 13

Original

    private readonly List<LightModel> lights = new()
   {
      new LightModel { Id = 1, Name = "Table Lamp", IsOn = false },
      new LightModel { Id = 2, Name = "Porch light", IsOn = false },
      new LightModel { Id = 3, Name = "Chandelier", IsOn = true }
   };

Updated

    private readonly List<LightModel> lights =
   [
      new LightModel { Id = 1, Name = "Table Lamp", IsOn = false },
      new LightModel { Id = 2, Name = "Porch light", IsOn = false },
      new LightModel { Id = 3, Name = "Chandelier", IsOn = true }
   ];

Why:

Simplifying collection initialization

Copy link
Contributor

Learn Build status updates of commit 304d7ac:

✅ Validation status: passed

File Status Preview URL Details
semantic-kernel/get-started/quick-start-guide.md ✅Succeeded

For more details, please refer to the build report.

For any questions, please:

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.

1 participant