search

Lakatrazz / BONELAB-Fusion

A multiplayer mod for BONELAB.
MIT License
121 stars 47 forks source link

Draft: Refactor project structure, code cleanup #46

Closed Meister1593 closed 8 months ago

Meister1593 commented 9 months ago

Reorganize solution structure to have 1 project = 1 folder Reformat, cleanup unnecessary code (unnecessary default values assignments, imports optimize)

Currently in draft as i'm still checking what to do and most importantly - while it compiles on my end on Linux under mono with msbuild 15, i haven't tried compiling this pr on windows (i'm going to do it on vm), and i haven't tested fully compiled dll with other players. Would also need users on windows, linux, mac with fusion helpers to check. Will also try to do github actions for project (would definitely require some tinkering and custom pipelines)

TrevTV commented 9 months ago

Most of these changes feel super unnecessary and feels like you just going through and letting the IDE auto-cleanup things it doesn't like. You also moved some things to NuGet packages without any additional work. They were embedded into the project to allow it to be all in a single DLL and not dealing with janky merging or unexpected version changes, as well as just making it much easier for a user to install it. You also seemingly removed the Steam API libraries from FusionHelper, those are required for it to function, not sure why you thought that was a good idea. Also, it's generally not a good idea to make such a large PR without discussing it with the primary developer, it's usually better to keep new PRs to fixing (smaller) things, rather than giant changes with no impact on functionality.

Meister1593 commented 9 months ago

Most of these changes feel super unnecessary and feels like you just going through and letting the IDE auto-cleanup things it doesn't like. You also moved some things to NuGet packages without any additional work. They were embedded into the project to allow it to be all in a single DLL and not dealing with janky merging or unexpected version changes, as well as just making it much easier for a user to install it. You also seemingly removed the Steam API libraries from FusionHelper, those are required for it to function, not sure why you thought that was a good idea. Also, it's generally not a good idea to make such a large PR without discussing it with the primary developer, it's usually better to keep new PRs to fixing (smaller) things, rather than giant changes with no impact on functionality.

I consulted with main dev and everything in this pr was the main intention - project cleanup, restructure. I don't deny autoformat or anything, that's the goal, just to make it a bit more appealing

I did not test it fully yet, thus why its a draft. I wasn't intending to leave it as is without any additional testing.

TrevTV commented 9 months ago

I was thinking more properly working with the dev instead of just asking if you could, I understand this is a draft, but there are a lot of would-be obvious things you missed, but probably wouldn't have if you had discussed it more with him.

Meister1593 commented 9 months ago

This is still highly WIP pr, and I intended to fully test it in any case. Nothing is gonna be even remotely close to be merged until it gets properly tested.

Meister1593 commented 9 months ago

Decided against of moving to nuget or submodules, they already kind of got changed and tailored for single bundle and other means of bundling inside of one dll wouldn't do much good or just way too complicated. Will proceed with other code checkup/cleanup

Meister1593 commented 8 months ago

Will close it since melon loader 0.6 update anyway gonna change project significantly, and this will no longer be relevant