garnet icon indicating copy to clipboard operation
garnet copied to clipboard

Specify and enforce code-style using .editorconfig rules

Open PaulusParssinen opened this issue 1 year ago • 5 comments

I'm opening this issue to get some input from the maintainers on the possibility of having the projects code-style enforced with .editorconfig rules.

Code-style such as naming conventions are very subjective and it would be nice if contributors could just do project wide dotnet format [--verify-no-changes] after doing their changes. This would reduce mental overhead of trying to deduce the correct naming for variables and fields (which are currently inconsistent, even across singular files in the project) and make future code reviews easier.

I'm most concerned about the inconsistency of the field and variable naming conventions and would like to suggest bumping all the dotnet_naming_rule.*.severity rules from suggestion to warning and then running dotnet format for the entire repository. This should avoid the manual work such as https://github.com/microsoft/garnet/pull/84.

Couple examples of naming inconsistencies

https://github.com/microsoft/garnet/blob/2aea8fb68f5c58e71652e31a2fafc216fb3e4be8/libs/storage/Tsavorite/cs/src/core/ClientSession/LockableContext.cs#L18-L19

https://github.com/microsoft/garnet/blob/2aea8fb68f5c58e71652e31a2fafc216fb3e4be8/libs/common/LightClient.cs#L31-L37

https://github.com/microsoft/garnet/blob/2aea8fb68f5c58e71652e31a2fafc216fb3e4be8/libs/storage/Tsavorite/cs/src/core/TsavoriteLog/TsavoriteLog.cs#L128-L150

Another thing that makes reading the code harder is the (inconsistenly) missing access and visibility modifiers from fields.

It's nice to see there is already effort to put into enforcing code-style already (see https://github.com/microsoft/garnet/pull/106 etc.) but the naming would be nice to get under control early on 😄

I am myself big fan of the .NET Runtime code-style + file scoped namespaces (applying file scoped namespaces can be done with Visual Studio to the entire solution, but probably wise to time so that it would not cause big conflicts with inprogress PRs)

PaulusParssinen avatar Mar 26 '24 11:03 PaulusParssinen

Hey @PaulusParssinen! Thanks for your input :) I 100% agree, we unfortunately got to working on code-style errors very close to the OSS release date, so we tried to avoid wide and potentially invasive changes. This is definitely on the to-do list!

TalZaccai avatar Mar 26 '24 17:03 TalZaccai

should probably be after #577 and maybe also #582

Meir017 avatar Aug 12 '24 07:08 Meir017

Quite sure the naming conventions are still all over the place, even after those PRs 😄

PaulusParssinen avatar Aug 12 '24 14:08 PaulusParssinen

@PaulusParssinen how many changes are there after making the required changes in the editorconfig and running dotnet format ?

Meir017 avatar Aug 12 '24 14:08 Meir017

how many changes are there after making the required changes in the editorconfig and running dotnet format ?

Not sure what you mean by this. I'm all for enabling more of the Roslyn analyzers (such as the collection expression change, just keep eye on the resulting IL & codegen because they're not always most optimal for projects like Garnet) but one of the primary requests of in this issue is fixing the inconsistent naming by enforcing dotnet_naming_rule.* and dotnet_naming_style.* rules across the repository. There is currently no such rules and if they are introduced, they should probably be introduced once there's small amount of PRs incoming to minimize conflicts.

PaulusParssinen avatar Aug 12 '24 20:08 PaulusParssinen