roflmuffin / CounterStrikeSharp

CounterStrikeSharp allows you to write server plugins in C# for Counter-Strike 2/Source2/CS2
https://docs.cssharp.dev
Other
742 stars 111 forks source link

Refactor config to use dotnet's own configuration and options #400

Closed frederikstonge closed 4 months ago

frederikstonge commented 4 months ago

Refactor config to use dotnet's own configuration and options.

This allows the user to separate his settings into multiple sections of options, use data annotations for validations of the config, and inject the configuration anywhere in the application.

Right now this is not final, it's more of a proposition.

  1. It has breaking changes
  2. I need to update the documentation on how to use the new configuration

Your input is welcome :)

frederikstonge commented 4 months ago

I really like the project and I would love to help put do some cleanup in the managed portion of the project.

Right now, there's a lot of static classes that uses static instances of other classes.

Starting from the bootstrap.cs file, I would make everything use DI, with interface and ready to be unit tested properly.

I would also recommend using some analyer nuget packages to make sure that the code stays clean:

I believe all three would be beneficial to this project :)

roflmuffin commented 4 months ago

I really like the project and I would love to help put do some cleanup in the managed portion of the project.

Right now, there's a lot of static classes that uses static instances of other classes.

Starting from the bootstrap.cs file, I would make everything use DI, with interface and ready to be unit tested properly.

I would also recommend using some analyer nuget packages to make sure that the code stays clean:

I believe all three would be beneficial to this project :)

Agreed, a lot of it comes down to backwards compatibility & history of the project. For most plugin authors, they are not coming from a C# & DI background, so the use of static classes offers a more familiar approach to authoring similar to SourcePawn plugins of old. The road to the solution would probably involve using DI interface based classes with static class wrappers.

There are substantial changes to be made that it would be worth considering creating a sizable breaking change release which fixes a bunch of items that are there purely for legacy reasons, i.e. bad vectors, schemas returning refs instead of just using setters/getters; methods not using automatically generated interfaces for schema classes etc.

frederikstonge commented 4 months ago

I really like the project and I would love to help put do some cleanup in the managed portion of the project.

Right now, there's a lot of static classes that uses static instances of other classes.

Starting from the bootstrap.cs file, I would make everything use DI, with interface and ready to be unit tested properly.

I would also recommend using some analyer nuget packages to make sure that the code stays clean:

I believe all three would be beneficial to this project :)

Agreed, a lot of it comes down to backwards compatibility & history of the project. For most plugin authors, they are not coming from a C# & DI background, so the use of static classes offers a more familiar approach to authoring similar to SourcePawn plugins of old. The road to the solution would probably involve using DI interface based classes with static class wrappers.

There are substantial changes to be made that it would be worth considering creating a sizable breaking change release which fixes a bunch of items that are there purely for legacy reasons, i.e. bad vectors, schemas returning refs instead of just using setters/getters; methods not using automatically generated interfaces for schema classes etc.

I'm wondering if having two different base class for plugin, BasePlugin, and another one with more DI. This way we could still be backward compatible.

KillStr3aK commented 4 months ago

I don't really see any benefit from switching the configuration manager other than the injectability it comes with, but that also could be done with having a reference to the config instance inside the PluginContext which is already injected

frederikstonge commented 4 months ago

I don't really see any benefit from switching the configuration manager other than the injectability it comes with, but that also could be done with having a reference to the config instance inside the PluginContext which is already injected

Why use a custom configuration manager when microsoft has a solution supporting data annotation validations, change monitoring, snapshots and more? I think we should use as much as of what the framework has to offer, it's less error prone and maintenance for us.