Refactor flank-scripts [PROPOSAL]

[jan-goral]

User story

As a developer, I want to have well-structured and described flank-scripts commands so I can clearly understand the purpose of each, also maintain and scale the module without a doubt.

Description of the problem

1. The packages structure is designed in a semi-random manner, which makes to code harder to scale and may cause additional inconsequences in the future.
2. The CLI that uses thirty part libraries is bounded with domain and occasionally contains logical operations that break SRP and separation of concerns.
3. The command hierarchy is not well structured and described.

Requirements

1. According to the separation of concerns principle, the logical part of the code should be separated from others.
2. The CLI layer that uses third-party libraries (CliKt) should be thin as possible and free of logical operations for increasing code portability.
3. The domain layer should be designed a flat as possible manner. Each feature should be exposed as a single pure function separated from others, that operates on a utility code.
4. Only non-domain code should be reusable so it's important to design a common package carefully.
5. Each command must have a description that explains the purpose of the command and the possible result of the call.

Proposed solution

There are 3 important layers (packages) for flank-scripts module:

1. cli. Thin non-logical layer, a glue between program arguments and domain logic. Also specifies command-line documentation.
2. ops. The domain layer contains logical operations. Should be clean, easy to understand, and straightforward.
3. util. A non-domain reusable layer that contains utility code. The API should be simple and declarative.

Working steps:

1. Separate logical operations from CliKt commands.
2. Design a new structure for commands.
3. Group CliKt commands in cli package and fix the structure
4. Fix descriptions of commands.
5. Separate common layer
6. Refactor common layer API
7. Group all domain code in ops package (ops as shorthand for operations)
8. Refactor and restructure ops package
9. Use a standardized naming convention for commands (camel-case preferred as easier to write).

[piotradamczyk5]

The packages structure is designed in a semi-random manner, which makes to code harder to scale and may cause additional inconsequences in the future.

Now they are organized by feature not semi-random.

I think that we should discuss it first, before making this kind of task

[jan-goral]

I think that we should discuss it first, before making this kind of task

This is a proposal, feel free to give any suggestion.

[jan-goral]

Now they are organized by feature not semi-random.

So tell me which one of the following:

ci
dependencies
exceptions
github
pullrequest
release
shell
testartifacts
utils

can be classified as a feature in the sense of meaning?

[piotradamczyk5]

Now they are organized by feature not semi-random.

So tell me which one of the following:

ci
dependencies
exceptions
github
pullrequest
release
shell
testartifacts
utils

can be classified as a feature in the sense of meaning?

According to Readme features are

ci
dependencies
pullrequest
release
shell
testartifacts

each package represents set of commands

exceptions
github
utils

should be reorganized to common package

if think that we could follow your proposition, but keep feature packages

<feature1>
    cli
    domain
    repo
<feature2>
    cli
    domain
    repo
...
common
    cli
    domain
    repo

WDYT?

[jan-goral]

@piotradamczyk5 The essence of flank-scripts are functions that are called by commands (Clikt commands). So my proposition was to group them in ops package in the following manner: (ops - operations IMO fits well for the domain of flank-scripts)

cli\
    ...
ops\
    ci\
        GenerateReleaseNotes.kt
        NextReleaseTag.kt
    git\
        LinkGitHooks.kt
    dependency\
        UpdateDependencies.kt
    firebase\
        GenerateJavaClient.kt
        UpdateApiJson.kt
        CheckForSDKUpdate.kt
    ios\
        InstallXcPretty.kt
        BuildEarlGreyExample.kt
    testartifacts\
        ...
    ...
util\
    git\
        ...
    github\
        ...
    zenhub\
        ...
    shell\
        ...
    ...

As in the example above: ops

• The packages structure inside ops should be flat.
• Each kt file in the ops package should contain only one public function which will be the source-level command. Besides that, the file can contain many domain-related private functions.
• The packages names in ops should be simple and generic [ci, git, firebase...]
• The functions in ops shouldn't be aware of the CLI library.
• The ops shouldn't contain files with functions that are not exposed as commands in clilayer.
• If the ops function requires some abstract dependencies like a repository object, they should be wrapped in structure and passed to function as a receiver (this inside function scope).

cli

• It is not required to keep package convention from ops.
• This is a boring presentation layer, should be easily replaceable, that's why it should be thin.
• The run method of CliktCommand should run no more than one of the public functions from the ops package.
• The only responsibility of CliktCommand implementation is to prepare arguments and dependencies (optionally) for the ops-function.
• the cli package should be kept separated from the ops because we can have another implementation for the presentation layer in the separated java module. For example with Gradle tasks instead of Clikt.

util

• The util should contain all reusable non-domain stuff necessary for the ops package.

In summary

With this convention, we can easily navigate through the essence of flank-scripts, which is inside the ops package where each kt file contains code related to only one command. This is acceptable for scripts, simple and clean.

Additionally

I do not recommend the repository pattern or other abstractions for scripts. It could be useful only for those commands which require different boundaries for different platforms, otherwise, that layer is useless, and ops can call util directly.

[jan-goral]

@axelzuziak-gogo @pawelpasterz @adamfilipow92 @Sloox WDYF?

[piotradamczyk5]

Hey @jan-gogo Great plan, I agree with almost all of the proposed changes, however, I really think that we should package by feature instead of packaging by layers. With packaging by features, you have a clear overview about the files which belong to feature, code is simpler, files are independent, changes are mostly made in a single package, moving/deleting feature is done quicker.

Please take a look at some articles about them:

• https://phauer.com/2020/package-by-feature/
• https://proandroiddev.com/structural-and-navigation-anti-patterns-in-modularized-android-applications-a7d667e35cd6
• https://proandroiddev.com/package-by-type-by-layer-by-feature-vs-package-by-layered-feature-e59921a4dffa
• http://www.javapractices.com/topic/TopicAction.do?Id=205

[jan-goral]

I know arguments about keeping feature-related layers together but IMO for our scripts, it is not convenient. We shouldn't litter clear domain package by keeping it together with nasty object-oriented CliKt API. Presentation code is not interesting because is simple as stupid, there is no logic there so we shouldn't see it often. It's just a wrapper for kotlin functions, a kind of bridge from the command line to the application. It's a noise from a feature perspective, super fast to write and remove. In my proposition, files inside the ops package also are independent, each must be independent of another, they can depend only on utils or third-party library code, so it's super clear, much faster to overview. All-important code is kept together, so you are not forced to navigate also thought not important code. Additionally, think about whole ops as a dependency for cli, and util as a dependency for ops [cli->ops->util]. Those could be even the separated modules.

Your proposition is good for those who are writing a complex application for a single platform like android, where UI is heavy and often a huge part of the feature.

[piotradamczyk5]

Ok let's go with your proposition 👍 Great job

[zuziaka]

Converted to #1406