[DISCUSSION] Refactor project architecture

[kroune]

Describe your feature request.

Current issue of the project I want to address:

1. Modules lack clear public api, which sometimes causes properties/handlers to become public by accident
2. When Module code becomes big it is harder to distinguish simple variable from user settings (those that you can change at clickgui), so when you see autoSpeed or forceSneak (in ModuleScaffold) it isn't clear whether it is a settings or a value
3. Modules directly depend on each other and can change each other state

it isn't obvious when variable can change from the code
4. Small change causes entire project to recompile. The project is basically a single module, project has ~136 000 lines of code, so sometimes relatively simple changes causes a long recompile time

My idea:
we can separate public api from Module implementation and keep Module entirely hidden

Example of possible way to implement this

// publicFacade (gradle module)

// Base classes:
class ModulePublicFacade {
    fun intRange() = null
}

// Actual Module
object ModuleScaffoldPublicFacade : ModulePublicFacade() {
    val something = intRange()
    var somePublicVariable = 0
}


// clientModules (gradle module)
// Base classes:

object ModuleManager {
    ...
}
sealed class ClientModule<T: ModulePublicFacade>(protected val publicFacade: T) {
    fun handler(lambda: () -> Unit) {
        println("handler")
    }
}


// Actual Module
internal object ModuleScaffold : ClientModule<ModuleScaffoldSettings>(ModuleScaffoldSettings) {
    private val a = handler {
        publicFacade.intRange()
        publicFacade.somePublicVariable++
    }
}

publicFacade (gradle module) doesn't depend on any other modules

clientModules (gradle module) depend on publicFacade(gradle module)
clientModules (gradle module) has every module marked as internal, only some other classes/objects can be exposed

main (gradle module) depends on publicFacade, clientModules (gradle modules)

Benefits of this separation:

1. Modules have a clear public facade, you will call it explicitly. Which makes it much easier to read/write code.
2. Modules can be prevented from exposing internal state in ci (using binary-compatibility-validator) in clientModules (gradle module), you would almost never need to expose/change smth public.
3. You rarely change user settings, most of the time you only change Module's code. That means that most of the time only a clientModules (gradle module) will be rebuilt and main module won't need to recompile. Which improves build times
4. Client modules don't depend on each other, only on public facade.

I know this would require a big refactoring and I am ready to make it myself

Additional context

No response

[sqlerrorthing]

but its bloats the codebase.

[sqlerrorthing]

kotlin isnt rust, which encourages this

[MukjepScarlet]

One interface which has one single implementation (forever), lol

GitHub workflow always rebuild the whole project including fetching maven dependencies.

[1zun4]

On 1./2., there is no good reason for us to have a public API on an all-in-one codebase. Since we also have always the pattern that user settings appear on the top of the class, and everything below, that is quite clear enough. Another thing is that your approach would not work with the even further nested Configurable's e.g. ToggleableConfigurable. At least not without creating even more Facades.

Regarding 3., Compile times were never an issue for me, so that is also not an argument, especially since it's being built from scratch when needed.
And about 4., I don't see how this would be beneficial apart from removing e.g. a Module from the code-base without removing it's settings.

I just can't agree with any of the ideas. I understand that you don't want Module's to depend on each other, but if that is the case, we can just introduce more Events.

[kroune]

On 1./2., there is no good reason for us to have a public API on an all-in-one codebase. Since we also have always the pattern that user settings appear on the top of the class, and everything below, that is quite clear enough.

public API will be still used inside of the project. May be it is better to call it explicit public surface. Big modules have a lot of private and public properties. Keeping user settings at the top is good, but I would argue it doesn't solve the issue in big ClientModules

It is easy to leave some variable public by accident. It is harder for reviewers to spot those issues in a big file that has a lot of both private and public fields. It can't be checked automatically easily. Which is why right now there are a lot of fields/methods that are public that shouldn't be.

If there is a separate file where publicly exposed fields are, it is easier for reviewer to spot them and it can be checked by abi validator in CI.

When you read a code block and see some variables you can't understand clearly (just from this code block) whether it is just variable that belongs to your ClientModule* and only changes from inside or if it is a property that other modules read/modify. You would have to look at declaration/usages which doesn't a lot of time, but it makes code harder to read and more mentally difficult to understand. In my approach if value is publicFacade.[something] it is public, if it isn't - it is local, belongs to your ClientModule and only it. You don't have to look anywhere/check anything else, it is clear from the block of code you are currently reading

Another thing is that your approach would not work with the even further nested Configurable's e.g. ToggleableConfigurable. At least not without creating even more Facades.

I guess you are talking about ChoiceConfigurable + Choice.
If so, public part of of Choice can be in the same file Module*PublicFacade. And implementation in the directory where ClientModule*, just may be separate file if need, just like now.

Regarding 3., Compile times were never an issue for me, so that is also not an argument,

I wasn't talking about gradle module, but ClientModule*. They depend on each other state and can even change it, currently it isn't obvious at all. ClientModules are big and it is harder too keep track of the things that are used by other ClientModules

And about 4., I don't see how this would be beneficial apart from removing e.g. a Module from the code-base without removing it's settings.

If gradle module is changed (for example moduleA), but abi is the same, gradle skips rebuilding of modules that depend on moduleA. So it only builds the module that has changes, which is a lot quicker

especially since it's being built from scratch when needed.

When working locally we reuse build cache and it would save some times in local development (Also I think cache should be also used in CI/CD, so I have made a pr for this)

I just can't agree with any of the ideas. I understand that you don't want Module's to depend on each other, but if that is the case, we can just introduce more Events.

I don't really understand how it would solve it, pls give more detailed explanation if you can

[kroune]

One interface which has one single implementation (forever), lol

I am not sure I understand what you mean, I didn't mention any interfaces, only explicit public surface

[kroune]

but its bloats the codebase.

I don't understand what this mean and why would it. Code gets clearly separated, which I think improves codebase readability, lowers coupling, allows to keep small public surface (and check it in CI). In some places you need to add 1 more word when you call property, but I wouldn't call this bloat

kotlin isnt rust, which encourages this

don't know what this means either