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.

Sign up for GitHub

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

Jump to bottom

.Net: Converted examples 52 and others to tests #4649

Merged
merged 8 commits into from
Jan 19, 2024
Merged

.Net: Converted examples 52 and others to tests #4649

merged 8 commits into from
Jan 19, 2024

Conversation

Krzysztof318
Copy link
Contributor

@Krzysztof318 Krzysztof318 commented Jan 17, 2024 •
edited
Loading

Motivation and Context

PR #4526

Description

Converted examples 52 and 75 to tests
Fix samples project files formatting based on common solution resharper settings.

Contribution Checklist

@Krzysztof318 Krzysztof318 requested a review from a team as a code owner January 17, 2024 10:38
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code documentation labels Jan 17, 2024
@github-actions github-actions bot changed the title Converted examples 52 and 75 to tests .Net: Converted examples 52 and 75 to tests Jan 17, 2024
@markwallace-microsoft markwallace-microsoft self-assigned this Jan 17, 2024
@markwallace-microsoft
Copy link
Member

markwallace-microsoft commented Jan 17, 2024 •
edited
Loading

@Krzysztof318 can you simplify this PR to remove all of the formatting changes? It will be easier to review and if we want to reformat the code that should be a seperate PR and the changes agreed with the team. I'd suggest creating an issue with the formatting changes you'd like to see.

@Krzysztof318
Copy link
Contributor Author

@markwallace-microsoft Hi, I can. Just need drop one commit.
But by the way, I would like to know why there are such restrictive resharper settings in the repo if they are not respected in many files anyway?

@Krzysztof318
Refactor variable name in AgentTools example
6d45ef4 
Updated the name of a private list variable in the AgentTools example to align with naming conventions. The 's_agents' name was adjusted to '_s_agents' and wherever this variable was used in the code, it has been replaced with its updated name.
@Krzysztof318
Rename variable for agent list
33474c9 
The variable `_s_agents` has been renamed to `_agents`.
@dmytrostruk
Copy link
Member

I would like to know why there are such restrictive resharper settings in the repo if they are not respected in many files anyway?

I think resharper settings are obsolete now. We use dotnet format to format the code. If you run dotnet build with Release configuration, dotnet format will be triggered automatically for you.

@Krzysztof318
Copy link
Contributor Author

@markwallace-microsoft formatting commit dropped

@Krzysztof318
Copy link
Contributor Author

@dmytrostruk I understand but it would be good to adjust the resharper settings file to be equal with .editorconfig or remove it, it causes a lot of red flags in the project

markwallace-microsoft
markwallace-microsoft approved these changes Jan 17, 2024
View reviewed changes
Copy link
Member

@markwallace-microsoft markwallace-microsoft left a comment

Choose a reason for hiding this comment

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

Just need to run dotnet format

@markwallace-microsoft markwallace-microsoft mentioned this pull request Jan 17, 2024
Open
@Krzysztof318
Refactor code to replace Console.WriteLine with this.WriteLine
2b632e2 
In multiple parts of the code, calls to `Console.WriteLine()` were replaced with `this.WriteLine()`.
@Krzysztof318 Krzysztof318 changed the title .Net: Converted examples 52 and 75 to tests .Net: Converted examples 52 and others to tests Jan 18, 2024
@Krzysztof318
Copy link
Contributor Author

Krzysztof318 commented Jan 18, 2024 •
edited
Loading

I fixed other samples. Btw. Example74_FlowOrchestrator is not convertible to tests due to used Console.ReadLine

@RogerBarreto RogerBarreto added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@RogerBarreto RogerBarreto added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@shawncal shawncal added the kernel Issues or pull requests impacting the core kernel label Jan 19, 2024
@RogerBarreto RogerBarreto added this pull request to the merge queue Jan 19, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 19, 2024
@RogerBarreto RogerBarreto added this pull request to the merge queue Jan 19, 2024
Merged via the queue into microsoft:main with commit f2fb71d Jan 19, 2024
18 checks passed
@Krzysztof318 Krzysztof318 deleted the examples-to-tests branch January 19, 2024 21:12
Bryan-Roe pushed a commit to Bryan-Roe/semantic-kernel that referenced this pull request Oct 6, 2024
@Krzysztof318 @RogerBarreto
.Net: Converted examples 52 and others to tests (microsoft#4649)
ebfbffe 
### Motivation and Context

<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
  1. Why is this change required?
  2. What problem does it solve?
  3. What scenario does it contribute to?
  4. If it fixes an open issue, please link to the issue here.
-->

PR microsoft#4526 

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

Converted examples 52 and 75 to tests
Fix samples project files formatting based on common solution resharper
settings.

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄

---------

Co-authored-by: Roger Barreto <19890735+RogerBarreto@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RogerBarreto RogerBarreto RogerBarreto approved these changes

@markwallace-microsoft markwallace-microsoft markwallace-microsoft approved these changes

Assignees

@markwallace-microsoft markwallace-microsoft

Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
Semantic Kernel
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Krzysztof318 @markwallace-microsoft @dmytrostruk @RogerBarreto @shawncal