Skip to content

21752-add-asDoit-and-asDoitIn-to-AST#1237

Merged
MarcusDenker merged 3 commits intopharo-project:developmentfrom
MarcusDenker:21752-add-asDoit-and-asDoitIn-to-AST
Apr 21, 2018
Merged

21752-add-asDoit-and-asDoitIn-to-AST#1237
MarcusDenker merged 3 commits intopharo-project:developmentfrom
MarcusDenker:21752-add-asDoit-and-asDoitIn-to-AST

Conversation

@MarcusDenker
Copy link
Member

arguments: { RBVariableNode named: 'ThisContext' }
body: (RBReturnNode value: self) asSequenceNode.

methodNode := self asDoitIn.
Copy link
Contributor

Choose a reason for hiding this comment

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

Will not it be better to introduce "asDoitIn: aContext" instead of no arg asDoitIn?
Looks like this place is the only sender.

Copy link
Member Author

Choose a reason for hiding this comment

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

but you do not need the aContext there... and more important: you might not even have it at the point where you call the method. (as this just compiles a method, that method later can take the context as an argument)

Copy link
Contributor

Choose a reason for hiding this comment

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

I just like how no arg evaluate looks now. And I thought that context version can be similar:

methodNode := self asDoitIn: aContext.
^methodNode generateWithSource valueWithReceiver: aContext receiver arguments: {aContext}.

And it leads me to think that #asDoItIn is not needed at all because the result is supposed to be used with context (does it?). Do you see other scenarios?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, ok. Yes, this is possible. I will do it in the next iteration. and the name should maybe be asDoItForContext:

@MarcusDenker MarcusDenker merged commit 9ea0d79 into pharo-project:development Apr 21, 2018
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