Specify and enforce code-style using .editorconfig rules

[PaulusParssinen]

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 #84.

Couple examples of naming inconsistencies

garnet/libs/storage/Tsavorite/cs/src/core/ClientSession/LockableContext.cs

Lines 18 to 19 in 2aea8fb

	readonly ClientSession<Key, Value, Input, Output, Context, Functions> clientSession;
	readonly InternalTsavoriteSession TsavoriteSession;

garnet/libs/common/LightClient.cs

Lines 31 to 37 in 2aea8fb

	readonly int BufferSize;
	readonly SslClientAuthenticationOptions sslOptions;
	Socket socket;
	/// <summary>
	/// Operation type
	/// </summary>
	public int opType;

garnet/libs/storage/Tsavorite/cs/src/core/TsavoriteLog/TsavoriteLog.cs

Lines 128 to 150 in 2aea8fb

	internal int logRefCount = 1;
	readonly ILogger logger;
	/// <summary>
	/// Whether we refresh safe tail as records are inserted
	/// </summary>
	readonly bool AutoRefreshSafeTailAddress;
	/// <summary>
	/// Callback when safe tail shifts
	/// </summary>
	public Action<long, long> SafeTailShiftCallback;
	/// <summary>
	/// Whether we automatically commit as records are inserted
	/// </summary>
	readonly bool AutoCommit;
	/// <summary>
	/// Whether there is an ongoing auto refresh safe tail
	/// </summary>
	int _ongoingAutoRefreshSafeTailAddress = 0;

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 #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)}

[TalZaccai]

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!

[Meir017]

should probably be after #577 and maybe also #582

[PaulusParssinen]

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

[Meir017]

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

[PaulusParssinen]

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.