-
Notifications
You must be signed in to change notification settings - Fork 54
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
expose the last evaluated result as it
in REPL
#734
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code and even the refactoring seem reasonable to me.
I just have no idea how the it
variable works in the code. I guess you grab the result from the finished REPL computation and set it in the base robot context?
@valyagolev , since this has merge conflicts, and now that you have push access, it's probably easiest to just close this PR and create a new one from a branch in the |
@byorgey in this case the conflict is small enough and should be easy to resolve. IIRC the only reason for creating branches in the swarm repo was the Restyled Action worked better that way or was there another reason? |
It just needs a rebase :) The branch is the continuation of the one
squash-merged. I will do the rebase when I wake up.
Thank you for the welcome!
…On Fri, Oct 7, 2022, 14:58 Ondřej Šebek ***@***.***> wrote:
@byorgey <https://github.com/byorgey> in this case the conflict is small
enough and should be easy to resolve.
IIRC the only reason for creating branches in the swarm repo was the
Restyled Action worked better that way or was there another reason?
—
Reply to this email directly, view it on GitHub
<#734 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABEVYS5O7ERPRVYNDUOVK3WCANAZANCNFSM6AAAAAAQ7JDAZM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yes, I'm sure fixing the merge conflicts will be easy, I just meant if some "administrative" work (such as rebasing) has to happen anyway, it could be an opportunity to reopen a PR from a swarm branch. Not required, just a suggestion.
That is one reason, but the other reason is that it makes collaboration easier. For example #669 was a very collaborative effort, and that is harder when PRs are opened from people's private forks where no one else can push. (Not that we are going to immediately start pushing commits to others' PRs! But as trust develops it becomes a possibility.) |
44c0aa1
to
5e33d99
Compare
I kept this branch & PR for this one to avoid duplicating the conversation. But next time I'll gladly push a new branch to the main repo directly. Thank you! The conflict and the comments were resolved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking nice! I just have a few comments about cosmetic issues.
We should probably also talk about the it
variable in the tutorial, but we can make a separate issue for that.
src/Swarm/Language/Pipeline.hs
Outdated
@@ -36,6 +37,11 @@ import Swarm.Language.Typecheck | |||
import Swarm.Language.Types | |||
import Witch | |||
|
|||
-- | A value, or something like a value, along with its | |||
-- type and requirements | |||
data Processed val = Processed val Polytype Requirements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What types will end up being substituted for val
? I think I saw Value
and Maybe Value
, will there be others?
I wonder if we can come up with a better name than Processed
. To me, the name doesn't really help me guess what the definition is. I guess you named it in parallel with ProcessedTerm
, but a ProcessedTerm
actually results from processing a Term
, whereas that's not the case for a Processed Value
. (Though perhaps we should rename ProcessedTerm
as well!) Also, seeing the type Processed
made me expect that ProcessedTerm
is now a type synonym for Processed Term
or something like that, but it's not.
Decorated
, perhaps? Typeful
? Typed
? I'm open to suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What types will end up being substituted for val? I think I saw Value and Maybe Value, will there be others?
I'd also not be too surprised if I saw there: ()
or Term
; [Var]
(e.g. as a list of suggestions for a typed hole)... It has a functorial feel to it. Obviously I'm not familiar enough with the codebase to have any idea if it's good or not. I also thought maybe there could be a way to have ProcessedTerm
be part of this already, but I'm not sure.
I wonder if we can come up with a better name than Processed.
Completely agree – it's more of a placeholder name, I was hoping for a feedback.
Actually I'm wondering why is it that there's such a hard separation between Type
and Requirements
. I guess they "mean" different things, but are they actually ever separated from each other anyway? Doesn't their calculation follow similar paths? Wouldn't they be essentially a decoration over a type in other PLs?
So with that in mind, I do like Typed
.
Maybe there could be a name for a generalised type like this, e.g... AssemblableType
?
then we could have:
data AssemblableType = AssemblableType Requirements Polytype
data Assemblable v = Assemblable AssemblableType v
or even:
data Assemblable v = AssemblableType Requirements Polytype v
type AssemblableType = Assemblable ()
And yeah, whatever we decide about the meaning of this should determine which file it goes to, as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like Typed
, that sounds most natural to me. I might consider using a record though:
data Typed v = Typed
{ _value :: v,
, _typed :: Polytype
, _requires :: Requirements
}
We use lenses a lot and this will be the result of ix
/at
so we might as well use lenses for this type too.
Btw. if you want to get feedback on something in PR, it is a good idea to insert some TODO
comments so that we don't forget about it. Alternatively, you can make comments on the code here on GitHub or in your editor if you have some plugin for that. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm wondering why is it that there's such a hard separation between Type and Requirements. I guess they "mean" different things, but are they actually ever separated from each other anyway? Doesn't their calculation follow similar paths? Wouldn't they be essentially a decoration over a type in other PLs?
Yes, very good observation. Right now requirements are calculated separately, after typechecking, but actually there are several bugs related to the fact that it doesn't work quite right (#540, #394) and I want to redesign things so that requirements are part of types, as they should be (#231).
But let's not get too crazy with refactoring in this PR (what you have so far is fine). I'd rather merge something that works to give us it
, and then contemplate further refactoring as a separate PR.
src/Swarm/Game/Robot.hs
Outdated
@@ -509,6 +513,28 @@ instance FromJSONE EntityMap TRobot where | |||
mkMachine Nothing = Out VUnit emptyStore [] | |||
mkMachine (Just pt) = initMachine pt mempty emptyStore | |||
|
|||
type instance Index RobotContext = Ctx.Var |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of adding Ixed
and At
instances for RobotContext
, but I would tend to put them right next to the definition of RobotContext
. Was there a particular reason for having them down here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just a bit lost in big files. Also I think the best placement for everything will get clearer if we clarify the specific types proposed here...
I'm also not very sure if I was right not to include the contents of the Store
...
There's another way to approach this particular change, namely by changing RobotContext
into a product of Ctx (Typed Value)
(or whatever we agree on) and the Store
. I do think the three Ctx
s should go together somehow. Three different maps potentially mean three different sets of Var
s: do we want them?
(Such a change will remove the need for those instances)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have any suggestions on how to split the Robot source file to be easier to orient yourself, feel free to chime in:
I did not mention this file in the list because it is only over 500 loc, but it is still in our top 10. 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another way to approach this particular change, namely by changing RobotContext into a product of Ctx (Typed Value) (or whatever we agree on) and the Store.
Yes, we contemplated this previously, in #190 . We abandoned it because it got too complex. But maybe if we try again we can find a simpler way to do it. Though again, I think we should save that for a different PR.
src/Swarm/TUI/Controller.hs
Outdated
|
||
startBaseProgram t@(ProcessedTerm _ (Module ty _) reqs _) = | ||
(gameState . replStatus .~ REPLWorking (Processed Nothing ty reqs)) | ||
. (gameState . robotMap . ix 0 . machine .~ initMachine t (topCtx ^. defVals) (topCtx ^. defStore)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it, I knew this code could be simplified somehow.
@valyagolev This is looking good so far. Let us know if you feel like you need more guidance to know how to proceed from here. |
what's the verdict on i will apply other suggested changes soon and merge. thanks! |
I think we agreed on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to work quite right when executing commands. For example, if I type build {move}
at the prompt, it says it1 := <r1> : robot
, which makes sense. But then when I type it1
at the prompt, it says it has type cmd robot
instead of robot
. And if I try to actually use it1
, it causes a fatal crash. We do have some special case behavior built in for things of cmd
type entered at the REPL, and I suspect we're running into that somehow, though I haven't yet figured out what's going on exactly.
We should also reset the replNextValueIndex
to 0 every time we start a new game (see the startGameWithSeed
function in Swarm.TUI.Model
).
@@ -361,6 +365,10 @@ robotsInArea o d gs = map (rm IM.!) rids | |||
rl = gs ^. robotsByLocation | |||
rids = concatMap IS.elems $ getElemsInArea o d rl | |||
|
|||
-- | The base robot, if it exists. | |||
baseRobot :: Traversal' GameState Robot | |||
baseRobot = robotMap . ix 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually I want to stop assuming that the base is always ID 0, and this refactoring is a great first step!
Co-authored-by: Brent Yorgey <[email protected]>
Oh, this I didn't test. Yeah I will test a few weirder scenarios, maybe we'll need a test for those... |
I think |
Ok, yes, now I see the problem and it requires making a decision. Basically if I run a line that returns If we store ghci stores
|
Yes, absolutely we want to store the returned By the way, relatedly, the syntax
looks strange, because |
I still think it's good to mark it differently somehow, even if the particular syntax is questionable |
Why? What difference does it make? |
@valyagolev I understand that it could be useful to make it different in order to more easily debug this new feature. A technically correct syntax would be:
Honestly the debug features I previously introduced were never useful after merging. But it may just be because that they required recompiling to enable - it would be better if we could e.g. turn them on in the Main Menu. I am fine with either way, but consider that making the result look different could be more confusing (GHCi does not do it either) and will require more code. 🤷 |
No, it's not really about debugging this feature, rather about avoiding
loss of information in REPL. If I run `some_func`, I probably also want to
see what exactly it returned, value-wise and type-wise ([there's][1] a very
useful `:set +t` in ghci). Otherwise there's no visual distinction between
two fundamentally different results. I'm sure the output syntax can be
tweaked.
That said, if the general opinion is still to hide the distinction between
`cmd a` and `a` in repl, I'll remove the extra code that shows it.
[1]:
https://downloads.haskell.org/~ghc/latest/docs/html/users_guide/ghci.html#ghci-options
…On Sat, Oct 15, 2022, 14:59 Ondřej Šebek ***@***.***> wrote:
@valyagolev <https://github.com/valyagolev> I understand that it could be
useful to make it different in order to more easily debug this new feature.
A technically correct syntax would be:
it0 = 0
it1 <- return 1
Honestly the debug features I previously introduced were never useful
after merging. But it may just be because that they required recompiling to
enable - it would be better if we could e.g. turn them on in the Main Menu.
I am fine with either way, but consider that making the result look
different could be more confusing (GHCi does not do it either) and will
require more code. 🤷
—
Reply to this email directly, view it on GitHub
<#734 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABEVYV3XN677UCACJA5NT3WDKTCHANCNFSM6AAAAAAQ7JDAZM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@valyagolev but there is no distinction between the values that you get:
Sure the code at the REPL line has different type, but you can see that in the corner and from the perspective of later lines the results are the same. 🤔 If you have some other difference in mind, please give an example where it would have an impact. I might be missing something. 😅 |
To offer a bit more perspective: there are essentially two different things you can do at the REPL. You can use it as a "comand center" where you enter instructions (i.e. things with a |
Also, I'm very open to thoughts/suggestions on whether the distinction between execution and evaluation could be confusing to players, and how we might make it less so. |
I pushed a commit to remove the discussed syntax thing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks @valyagolev for your hard work on this!
Now we just need to implement #60 so we can actually see the previous it{n}
bindings!
@valyagolev in the future remember you can merge by adding the |
oh right, sorry, forgot about that |
This depends on #733.
This partially addresses #304.
There's one essential commit with the functionality: 8666e55 . And then there's a small refactoring that I'm proposing, but since I'm unfamiliar with the codebase, I have no idea if it's useful or not.