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 module for CESK store #1730

Closed
wants to merge 1 commit into from
Closed

split module for CESK store #1730

wants to merge 1 commit into from

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Jan 16, 2024

Towards #1715 and #1043.

A follow-on to #1729; this refactoring step is a prerequisite for #1719 to extricate references to the CESK module from the base RobotR definition.

Motivation

Originally tried in #1731 to just remove dependence of TRobot on the RobotContext record, since RobotContext depends on Store which is defined in the CESK module However, this attempt did not work. So instead, we split the definition of Store out of the CESK module and allow TRobot to continue to depend on RobotContext.

@kostmo kostmo marked this pull request as ready for review January 16, 2024 00:32
@kostmo kostmo requested a review from byorgey January 16, 2024 00:32
@byorgey
Copy link
Member

byorgey commented Jan 16, 2024

So is the idea that by splitting out this module we can keep Swarm.Game.CESK.Store in the scenario construction library with Robot, and Swarm.Game.CESK would go in the swarm-engine library? I don't like that, since, again, Store really has nothing to do with scenario construction and everything to do with the runtime system, and there's no reason a template robot should ever need to have a Store. In other words I'm not sure this is the correct place to do the split.

Looking at the definition of RobotContext, I think perhaps the Store really doesn't belong there. Everything else in the RobotContext stores information about defined names; notice the At instance, which returns the type, value, and requirements for a name (using the first three components of a RobotContext) but has nothing at all to do with the defStore. Although it would be more work, I think perhaps the correct way to do this is to split the defStore out of the RobotContext and store it in the Robot record separately, but using a type family so that it stores () in a TRobot and an actual Store in a concrete Robot. There is no reason a template robot should ever need to have a Store.

By the way, all this refactoring is annoying but I'm actually really happy with this process so far, and I really appreciate your work here. Splitting into sublibraries like this is really forcing us to think through some things that are too tightly coupled, and it's resulting in refactored code that is cleaner and more sensible.

@kostmo kostmo marked this pull request as draft January 16, 2024 15:40
@kostmo kostmo closed this Jan 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants