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.

Sign up for GitHub

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

Jump to bottom

Remove superfluous trobotContext lens #1731

Merged
merged 2 commits into from
Jan 16, 2024
Merged

Remove superfluous trobotContext lens #1731

merged 2 commits into from
Jan 16, 2024

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Jan 16, 2024 •
edited
Loading

Motivation

Want to remove dependence of TRobot on the RobotContext record. However, a lens trobotContext had been added in #817 to fix #394.

Test

scripts/run-tests.sh --test-arguments '--pattern "394-build-drill"'

@kostmo kostmo added the Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. label Jan 16, 2024
@kostmo kostmo mentioned this pull request Jan 16, 2024
Closed
byorgey
byorgey reviewed Jan 16, 2024
View reviewed changes
src/swarm-engine/Swarm/Game/Step/Const.hs Outdated
@@ -1061,7 +1061,7 @@ execConst runChildProg c vs s k = do
-- Construct the new robot and add it to the world.
parentCtx <- use robotContext
newRobot <-
zoomRobots . addTRobot (In cmd e s [FExec]) . (trobotContext .~ parentCtx) $
zoomRobots . fmap (robotContext .~ parentCtx) . addTRobot (In cmd e s [FExec]) $
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason this doesn't work is that it only modifies the robotContext of the Robot value returned by addTRobot, but it does not modify the original Robot value which addTRobot actually adds to the robot map (of which it then returns a copy).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could just have addTRobot take a Maybe RobotContext as an extra argument? I don't know if that's actually a good idea, just random brainstorming...

@kostmo kostmo marked this pull request as ready for review January 16, 2024 15:43
@kostmo kostmo requested a review from byorgey January 16, 2024 15:43
byorgey
byorgey approved these changes Jan 16, 2024
View reviewed changes
Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, wonderful. I like how this uncovers the fact that the only time we really needed the returned Robot was when we were actually setting the context, so now in the common case we can have addTRobot return () and get rid of a bunch of calls to void.

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Jan 16, 2024
@mergify mergify bot merged commit 40ea471 into main Jan 16, 2024
9 checks passed
@mergify mergify bot deleted the refactor/trobot-context branch January 16, 2024 17:36
@kostmo kostmo mentioned this pull request Jan 16, 2024
Merged
mergify bot pushed a commit that referenced this pull request Jan 20, 2024
@kostmo
Use type family for RobotContext field (#1732)
9320a98 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@byorgey byorgey byorgey approved these changes

Assignees
No one assigned
Labels
merge me Trigger the merge process of the Pull request. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Inner build does not correctly require Drill
2 participants
@kostmo @byorgey