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

C#: Reorganize Structure #3362

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

X39
Copy link

@X39 X39 commented Mar 12, 2025

See #3354

This is part one of the split PR

@X39 X39 requested a review from a team as a code owner March 12, 2025 23:34
@X39 X39 force-pushed the csharp/reorganization branch 3 times, most recently from 2b15406 to 406a658 Compare March 12, 2025 23:57
@X39
Copy link
Author

X39 commented Mar 12, 2025

forgot to update from upstream first, sorry

@Yury-Fridlyand Yury-Fridlyand added the C# C# wrapper label Mar 13, 2025
@X39 X39 force-pushed the csharp/reorganization branch from e4d5908 to 7d34e85 Compare March 13, 2025 00:56
Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

I think you need to wait a bit until we merge other PRs which are on the finish strech and then rebase your code.
#3347 #3321 #3321 and #3335

csharp/ReadMe.md Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to rename. We will have a developer guide and a user guide, they are different.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also not sure PascalCase naming for readme files works. At least it is first time I see it.
I would say it goes agains common patterns and guidelines even for .NET ecosystem

https://devblogs.microsoft.com/nuget/add-a-readme-to-your-nuget-package/
https://visualstudiomagazine.com/articles/2017/02/21/vs-dotnet-code-documentation-tools-roundup.aspx

Comment on lines 267 to 268
async (key, value) => await glide_client.Set(key, value),
() => glide_client.Dispose()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I afraid whitespace changes would be blamed by the linter

@@ -324,4 +322,4 @@ public static async Task Main(string[] args)

PrintResults(options.ResultsFile);
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please keep blank lines at EOF

@@ -1,6 +1,6 @@
// Copyright Valkey GLIDE Project Contributors - SPDX Identifier: Apache-2.0

namespace Tests.Integration;
namespace Valkey.Glide.UnitTests.Integration;
Copy link
Collaborator

Choose a reason for hiding this comment

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

All tests there are truly integration tests.

Changes include accidental whitespace changes and a rename that does not yet make any sense. Also reverted naming of DEVELOPER.md in accordance with contributors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C# C# wrapper
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants