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

up c# syntax, make cli defaults in one style #903

Merged
merged 11 commits into from
Feb 7, 2024

Conversation

Sa1Gur
Copy link
Contributor

@Sa1Gur Sa1Gur commented Feb 5, 2024

Updating c# syntax

  • file scope namespace
  • arrow method/property body
    Making defaults for CommandLineOptions in one style (using attribute parameter Default)

@Sa1Gur Sa1Gur requested a review from a team as a code owner February 5, 2024 19:10
@Yury-Fridlyand Yury-Fridlyand added the C# C# wrapper label Feb 5, 2024
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.

Do you want to introduce C# linter which will lint all code automatically?
And/or you can create .editorconfig file

@Sa1Gur
Copy link
Contributor Author

Sa1Gur commented Feb 6, 2024

Added .editorconfig. I can also add dotnet format step to CICD

@shachlanAmazon
Copy link
Contributor

please do add linting in the C# CI - style changes should be accompanied by a linter that will catch such style violations in the future.
Additionally, please rebase over #904 - this PR introduced new warnings to the code, and by the same rule, I want the CI to catch such warnings.

.editorconfig Outdated
tab_width = 4

# New line preferences
end_of_line = crlf
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not lf? Are you changing all files to lf or crlf?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no preferences. Making it lf

.editorconfig Outdated

# New line preferences
end_of_line = crlf
insert_final_newline = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

conflicts with line 6. Please keep true there.

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 is not conflict - it is just specific settings for cs. Changing to true

@shachlanAmazon
Copy link
Contributor

@Yury-Fridlyand any further comments?

@Yury-Fridlyand Yury-Fridlyand merged commit c531f86 into valkey-io:main Feb 7, 2024
3 checks passed
@shachlanAmazon
Copy link
Contributor

@Sa1Gur we'd love to know whether you're planning on advancing the C# prototype and move towards a full working wrapper, or whether you were just interested in fixing the existing code.

@Sa1Gur
Copy link
Contributor Author

Sa1Gur commented Feb 7, 2024

Yes, I'm planning to advanced C# prototype.

@Sa1Gur Sa1Gur deleted the syntax_up branch February 13, 2024 15:28
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.

4 participants