LeagueSandbox / GameServer

League Sandbox's Game Server
GNU Affero General Public License v3.0
1.07k stars 432 forks source link

Swap the CSharpScriptingEngine for the Nuget package #681

Open LeagueSandboxBot opened 6 years ago

LeagueSandboxBot commented 6 years ago

I put the scripting engine into its own Nuget package. We can use that as a Nuget library.


Beep, boop, I'm a bot! This issue was created by @Gan in #development.

MythicManiac commented 6 years ago

I'm a bit doubtful on this one, as the scripting engine is so closely coupled to the game server. Will have to discuss this more

MatthewFrench commented 6 years ago

https://github.com/MatthewFrench/CSharpScriptingEngine/blob/master/CSharpScriptingEngine/CSharpScriptingEngine/CSharpScriptEngine.cs

Here’s the CSharpScriptEngine class. It doesn’t have any dependency on GameServer, making it completely decoupled in a code sense. Even though it was made with only GameServer in mind, it can easily be dropped in any project as a scripting engine. We can keep it in GameServer because it’s only a single file or we can remove it. It won’t have any serious impact.

MythicManiac commented 6 years ago

Hmm, my personal opinion is that we probably shouldn't extract it as far as a nuget package at this point at least.

This is because it's such a small piece of code that's not in the way of anything, and at least I can see it possibly being edited later on. If it's a NuGet package, it becomes a lot more difficult to make edits.

I do see the thinking behind this, and were the package itself bigger or guaranteed to basically never change, I'd think this is a good idea to do now.

I'll reconsider if this gets suport from more people, but for the time being I don't think this is the best choice.

danil179 commented 3 years ago

The scripting engine actually changed frequently for implementing more features in it. I think that a NuGet package is the way to go, but seems like that keeping it in the files make it much easier to update/add new features. I think that it shouldn't be in another NuGet in this case.