enowars / EnoEngine

MIT License
12 stars 2 forks source link

Move checker-related stuff to its own repository #161

Open ldruschk opened 3 years ago

ldruschk commented 3 years ago

In my opinion, there is no reason why the C# EnoChecker is placed in the EnoEngine repository. Moving this to its own repository would help clarifying which part of the code are necessary to interface with any checker of the specified interface and which parts are relevant only to the checker server implementation in C#.

If there is any re-used code between the engine and C# checker, that should be cleanly placed in its own package.

DanielHabenicht commented 3 years ago

yep would be cool to have it in a enochecker-dotnet repository and renaming the other checkers accordingly, e.g. enochecker (which is python) to enochecker-python

Trolldemorted commented 3 years ago

I agree that a refactoring is necessary. Our requirements are:

Therefore we could split our packages in the following way:

DanielHabenicht commented 3 years ago

IChecker and friends follow EnoChecker and form a new project, e.g. EnoChecker.Core.

Are you sure you want another package for Enochecker? I would just publish EnoChecker as it is in the new Repository. I think we both mean the same: Publishing EnoChecker as a new package.

DanielHabenicht commented 3 years ago

might it be useful to have PRs for refactoring this stuff? I think you have a typo in here: https://github.com/enowars/EnoEngine/commit/9c7139fde73c5056ab834a00f81907760402174b#diff-9bc0aa9f51ff6aedff652c211fa7771aff85093ba5c42432e263d0565c8b9e34R1

And in my last PR we talked about that everything should be LF, but now we are checking for CRLF? https://github.com/enowars/EnoEngine/commit/9c7139fde73c5056ab834a00f81907760402174b#diff-0947e2727d6bad8cd0ac4122f5314bb5b04e337393075bc4b5ef143b17fcbd5bR8

Whats about the dummy checker should we retain it somewhere for performance tests?

Trolldemorted commented 3 years ago

I think you have a typo in here: 9c7139f#diff-9bc0aa9f51ff6aedff652c211fa7771aff85093ba5c42432e263d0565c8b9e34R1

oh thanks, I guess we'll tend to that after enowars 5 is over though

And in my last PR we talked about that everything should be LF, but now we are checking for CRLF? 9c7139f#diff-0947e2727d6bad8cd0ac4122f5314bb5b04e337393075bc4b5ef143b17fcbd5bR8

No, you just commited a handful of CR files into an otherwise completely CRLF repo

Whats about the dummy checker should we retain it somewhere for performance tests?

yeah we should keep it with enochecker and provide prebuilt docker images, but that shouldn't be much work