Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split Swarm components into libraries #1043

Closed
xsebek opened this issue Jan 24, 2023 · 8 comments · Fixed by #1834
Closed

Split Swarm components into libraries #1043

xsebek opened this issue Jan 24, 2023 · 8 comments · Fixed by #1834
Labels
C-Low Hanging Fruit Ideal issue for new contributors. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.

Comments

@xsebek
Copy link
Member

xsebek commented Jan 24, 2023

I propose we split the sources into Cabal libraries.

Practical problem: compilation

When I run tests after changing some Swarm.Language modules, every Swarm source file (60+) is recompiled.

However, we do not need to compile LSP or TUI or Web API, because we do not test them.

Theoretical problem: dependencies

We mostly* avoided using TUI or Web dependencies in the code for language and game logic.

In theory, we could switch the dependencies for new ones or add another UI without changing the game or language code.

Left unchecked, the dependencies could slowly leak and make future experiments less viable.

At least it would be easier to see which dependencies are needed for what part of the Swarm application.

Solution

In my mind, Swarm code is split like this in order of dependency:

  • Swarm.Language
    • Swarm.Language.LSP (move to its own directory?)
    • Swarm.Game
      • Swarm.DocGen (not really required in the final application)
      • Swarm.TUI
        • Swarm.Web

Swarm.App is just a small module to neatly initialise everything in order.

There is also Swarm.Version, which has a failure type that we use in TUI to show it nicely.
We could split off the failure type and use the network call and Git polling only in App.


*) except for Brick AttrName in Swarm.Game.Display.

@xsebek xsebek added Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. C-Low Hanging Fruit Ideal issue for new contributors. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. labels Jan 24, 2023
@byorgey
Copy link
Member

byorgey commented Jan 24, 2023

Makes sense to me, as long as we continue to have everything in a single repository.

@xsebek
Copy link
Member Author

xsebek commented Jan 24, 2023

@byorgey repository definitely stays the same. 🙂

The simple idea is to make directories directly in src libraries to get a small compilation speedup and a good feeling from doing The Right Thing ™️

@kostmo
Copy link
Member

kostmo commented Jan 31, 2023

If you're going to start on this, I wonder if we could please merge #873 in a partially-complete state first, since the merge conflicts would probably be massive.

@xsebek
Copy link
Member Author

xsebek commented Feb 1, 2023

@kostmo good point. I think we can try another approach first.

I will take file content changes out of the PR and merge them separately.

If I am patient enough with that, then the final PR will be only moving files into subdirectories and the cabal file.

mergify bot pushed a commit that referenced this issue Feb 9, 2023
- Move the Location module to Game
- Move Achievements modules to Game
- Move Render and Model modules to TUI
- split off from #1069
- part of #1043
mergify bot pushed a commit that referenced this issue Feb 10, 2023
This avoids using `Brick.AttrName` and we can remove the `-fno-warn-orphans` from `Attr.hs`.

As a bonus, we can now specify a terrain attribute (`terrain_stone`) in a scenario along with the `default` attribute.

- split off from #1069
- part of #1043
mergify bot pushed a commit that referenced this issue Feb 13, 2023
- remove the useless `DirInfo` type
- move the `Direction -> Heading` functions to the `Location` module
- split off from #1069
- part of #1043
- part of #707
@byorgey
Copy link
Member

byorgey commented Nov 19, 2023

@xsebek I'm curious what your thoughts are on this now. We had #1069 but then you closed it and I'm not sure why. (If it's because you didn't get enough response/reviews, sorry about that!) I still think this could be a good idea, though I'm still not totally sure I understand all the implications.

@xsebek
Copy link
Member Author

xsebek commented Nov 19, 2023

@byorgey not at all! 🙂 It was simply taking time to rebase the PR, given that it moved around lot of code.

I think this is a good thing ™️ to do, but needs a concrete immediate use-case. For example deciding that LSP client/Doc generator or some other part should be moved into a separate executable and depend on "swarm the language" library. Then the short term annoyance of investigating what could be broken by this change should get outweighed by the benefits. 😅

@kostmo
Copy link
Member

kostmo commented Nov 20, 2023

Would it be more feasible (with regard to rebasing) to tackle one sub-library at time (i.e. peel off one library per PR)?

For starters, perhaps just the Swarm.Util module could be separated and used as a testbed for separating more libraries.

@byorgey
Copy link
Member

byorgey commented Nov 20, 2023

Yes, that sounds very reasonable to me.

mergify bot pushed a commit that referenced this issue Dec 7, 2023
Towards #1043

This is the first of potentially more granular sub-library splits.
@kostmo kostmo mentioned this issue Dec 30, 2023
mergify bot pushed a commit that referenced this issue Jan 1, 2024
Towards #1043

Creates a new sub-library for the web service.  Also moved the `Swarm.App` module from the `swarm` library to the `swarm` executable.
This was referenced Jan 3, 2024
mergify bot pushed a commit that referenced this issue Jan 5, 2024
Towards #1043

Extracts `Swarm.Game.*` modules to their own sublibrary.

There was already pretty good separation along this boundary; just had to move three functions into a new module `Swarm.Util.Content`.
mergify bot pushed a commit that referenced this issue Jan 14, 2024
Towards #1715 and #1043.

This refactoring step is a prerequisite for #1719 to extricate references to the `CESK` module from the base `RobotR` definition.

In this PR:
* `Swarm.Game.CESK` is imported qualified to more easily track usages
* A new `RobotMachine` type family is added to parameterize the `_machine` field.
* `CESK` is a new parameter to `addTRobot`
mergify bot pushed a commit that referenced this issue Jan 20, 2024
Continuation of #1731.

Towards #1715 and #1043.

This refactoring step is a prerequisite for #1719 to extricate references to the `CESK` module from the base `RobotR` definition.

# In this PR:

* Extracts the `RobotContext` type to a new module
* Introduces a type family for the `RobotContext` field
mergify bot pushed a commit that referenced this issue Jan 24, 2024
Towards #1715 and #1043

Creates a new `swarm-scenario` sublibrary intended for scenario data that is independent of game state.

# Planned follow-ups

This PR is already pretty large, but there is still more that can be done regarding sublibrary reorganization/splitting.

* May want to pare-down a sublibrary exclusively for world-generation without all the other baggage of scenarios.
* `Swarm.Game.ScenarioInfo`, `Swarm.Game.Tick`, and `Swarm.Game.Scenario.Status` could probably be moved from `swarm-scenario` to `swarm-engine`.
@mergify mergify bot closed this as completed in #1834 May 11, 2024
mergify bot pushed a commit that referenced this issue May 11, 2024
Split the final remaining default library into `swarm-doc` and `swarm-tui`.  Closes #1043 .
mergify bot pushed a commit that referenced this issue Jun 2, 2024
Towards #1043.

The eventual goal of this sublibrary split is to have a self contained library that can compose 2D grids of arbitrary content (perhaps colored pixels, or boolean values).  This could be useful outside of the `swarm` game.

I would also like to write unit tests for the structure recognizer that are independent of the `Entity` type.

# Major Changes

## Direction module
* Moved `Swarm.Language.Syntax.Direction` to `swarm-util`, since both `swarm-lang` and `swarm-topology` depend on it, but not on each other.
* Removed the re-export of direction things from `Swarm.Language.Syntax`

## Structure module

The `Swarm.Game.Scenario.Topography.Structure` module has been split into two:
* `Swarm.Game.Scenario.Topography.Structure`
* `Swarm.Game.Scenario.Topography.Structure.Type`

The former retains the YAML parsing logic.  The latter is agnostic of `Enitiy` type and the palette .
At some future point, I might want to move the YAML parsing to this sublibrary while still retaining independence of `Entity` type.

## Structure recognizer

The structure recognizer is independent of the content of Cells (i.e. it does not need to know what an `Entity` is), except:
1. during initialization
2. when retrieving the original cell content after recognition

Type parameters for three kinds of data have been added to the recognizer:
1. `Cell`/`PCell`
2. `Entity`
3. `EntityName`

Eventually it may be possible to eliminate one or two of these type parameters, with some refactoring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Low Hanging Fruit Ideal issue for new contributors. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Developer Experience This issue seeks to make life easier for developers writing Scenarios or other Swarm code. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
3 participants