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 large Haskell modules #707

Open
xsebek opened this issue Sep 27, 2022 · 1 comment
Open

Split large Haskell modules #707

xsebek opened this issue Sep 27, 2022 · 1 comment
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.

Comments

@xsebek
Copy link
Member

xsebek commented Sep 27, 2022

Some of the Swarm Haskell modules have grown exceedingly large, which makes them hard to maintain.

Notably:

find src -name '*.hs' -exec wc -l {} \; | sort -h | tail -n 6
#  857 src/Swarm/Game/State.hs
#  897 src/Swarm/Language/Syntax.hs
#  942 src/Swarm/TUI/Controller.hs
#  999 src/Swarm/TUI/Model.hs
# 1130 src/Swarm/TUI/View.hs
# 2008 src/Swarm/Game/Step.hs

It also makes compilation slightly longer, since any change causes the recompilation of the entire file and its dependencies.

Proposed solution

The Step module is the largest one and the trickiest to split. But I think it could be reasonably split into three parts:

  • Exec: execConst function which currently accounts for ~1000 loc
  • Step: running CESK machines (stepCESK, evalPT,...)
  • various utility functions - some of them are general enough to go in some Util module, others could be moved to Robot or GameState

I think we could make room for the utility functions on robots and game state if we moved the records themselves into an internal module.

For example, if we moved the GameState record and its lenses:

module Swarm.Game.State.Internal (GameState, creativeMode, ...)
module Swarm.Game.State (module Swarm.Game.State.Internal, initGameState, scenarioToGameState, ...)
import Swarm.Game.State.Internal

This could of course be tweaked for documentation so that the Step module renders nicely.

@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. labels Sep 27, 2022
mergify bot pushed a commit that referenced this issue Oct 29, 2022
When the inventory pane is selected (Alt+e), one can use the `;` and `:` keys to cycle sort criteria and sort direction, respectively.
![Screenshot from 2022-10-26 21-52-11](https://user-images.githubusercontent.com/261693/198195262-e0ba6f22-cdda-4a29-bc99-398f721a1ed5.png)

This functionality is implemented in a new module to avoid exacerbating #707.

Note that `entityProperties` may be an unreliable criteria to sort, because it is a list that itself may contain duplicates or items in unpredictable order.  Perhaps this field should be made a `Set` (#794).
mergify bot pushed a commit that referenced this issue Dec 12, 2022
Partially towards #707, although `Scenario.hs` is not that large.  Primarily this refactoring is in anticipation of #873 (see [comment](#873 (comment))).

Just like #879, there are no functional changes in this PR---only code relocation.
mergify bot pushed a commit that referenced this issue Dec 12, 2022
Towards #707.

Halves the size of `Model.hs` by splitting into smaller modules.

There are no functional changes in this PR---only code relocation.
mergify bot pushed a commit that referenced this issue Dec 12, 2022
Just like #880, this refactoring is in anticipation of #873 (see [comment](#873 (comment))) for ease of review, and there are no functional changes in this PR.

Also towards #707.
mergify bot pushed a commit that referenced this issue Dec 14, 2022
Also fixes module pluralization according to [this comment](#880 (comment)).

This is a no-op refactoring, only involving renaming/code relocations.

Towards #707.
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 byorgey added C-Moderate Effort Should take a moderate amount of time to address. and removed C-Low Hanging Fruit Ideal issue for new contributors. labels Feb 18, 2023
mergify bot pushed a commit that referenced this issue Jun 9, 2023
Towards #558
I was motivated to build this after finding that editing scenario maps directly in the YAML file is rather constraining.

## What I've implemented so far
* A small, collapsible panel to the left of the REPL containing World Editing status/operations.  Enter world-editing mode with CTRL+e to show the panel.
    * This works only in `--cheat` mode
* Terrain selection
    * A "picker"/"eye dropper" middle-click mechanism to select a terrain style to draw.
    * A pop-up selector to choose between the 5 different types of terrain.
* Drawing terrain with the left mouse button
* Saving a rectangular section of the world map (terrain only) to a file with CTRL+s
* Code organization
    * The complete state of the World Editor, including "painted overlays" of terrain, is contained within the `uiWorldEditor` field of `UIState` record.
    * The bulk of the World Editor functionality shall be in new modules
    * Some refactoring of `Controller.hs` and `View.hs` to extract functions utilized by the World Editor (towards #707)

## Vision

* The audience for this tooling is strictly envisioned to be Scenario authors.
    * Though, if we eventually allow swarm-lang to program the UI, there may be some common code to extract.
* The World Editor is intended to be compatible with a workflow of editing maps in text form within YAML scenario files.

# Demos
## Round-trip with random world

    stack run -- --scenario creative --seed 0 --cheat

Then Ctrl+e, tab down to the Save button, hit Enter to save the map
In another tab run:

    stack run -- --scenario mymap.yaml

Toggle between tabs to compare, observe the derived map is an identical 41x21 subset.
@byorgey
Copy link
Member

byorgey commented Dec 11, 2023

Update:

diophantus :: ~/projects/swarm » find src -name '*.hs' -exec wc -l {} \; | sort -h | tail -n 6
852 src/Swarm/Game/Step.hs
1126 src/Swarm/Language/Typecheck.hs
1208 src/Swarm/Language/Syntax.hs
1467 src/Swarm/TUI/View.hs
1503 src/Swarm/TUI/Controller.hs
1693 src/Swarm/Game/Step/Const.hs

We managed to split up Step, though Step.Const is still the longest module. Maybe the next place to focus some energy would be the TUI modules as well as syntax + typechecking.

mergify bot pushed a commit that referenced this issue Jul 14, 2024
Towards #707

Reduces `Swarm.Game.State` by 200 lines and better encapsulates non-exported initialization functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Moderate Effort Should take a moderate amount of time to address. S-Nice to have The bug fix or feature would be nice but doesn't currently have much negative impact. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Development

No branches or pull requests

2 participants