-
Notifications
You must be signed in to change notification settings - Fork 40
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
Preserver previous command results (avoid rerunning commands) #128
base: master
Are you sure you want to change the base?
Conversation
feat: save current result as a result for current command
Well this is interesting! I'll come back to review a bit later, but my first question is what is the difference between |
Well difference between puts "previous value: #{_p}" Btw: I tried to replace |
I'm not sure that I follow exactly. I know this works in Crystal __ = 1
__ += 1
puts __ Here's the PR where it was added in originally #63 I just want to make sure that if we're making this change, it's not a 1 to 1 of what we have now for the sake of "a better name" or something along those lines. I'd rather not have 2 ways to do this, so if we're going to add in |
@jwoertink you were right! Sorry my mistake. I was too tired yesterday then I could not test it very well. |
No worries! I just want to make sure we're getting this right. Thanks for contributing ❤️ |
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.
Sorry it took me some time to come back and review this. It seems like a pretty decent start. I was able to create a time object and it persisted between calls which is an amazing improvement. It looks like there's still some issues though.
For example:
icr(1.0.0) > t = Time.utc
=> 2021
icr(1.0.0) > t
=> 2021
icr(1.0.0) > t.to_s("%F")
=> "2021-06-14"
It seems the default output of a Time object is just the year which could be a bit confusing.
Trying to use both __
and __previous_result
just return this error:
icr(1.0.0) > __
Showing last frame. Use --error-trace for full trace.
In .icr_Vbx6IjLdROuGGOgDEDzfUA.cr:14:3
14 | __previous_result = (__previous_result);
^
Error: expression has no effect
icr(1.0.0) > __previous_result
Showing last frame. Use --error-trace for full trace.
In .icr_Vbx6IjLdROuGGOgDEDzfUA.cr:14:3
14 | __previous_result = (__previous_result);
^
Error: expression has no effect
Lastly, trying to define a simple class in two lines seems to bork the whole session:
icr(1.0.0) > class Test
icr(1.0.0) > end
There was a problem expanding macro 'macro_4538303136'
Code in macro 'init'
1 | {% if T < Enum %}
^
Called macro defined in macro 'init'
1 | {% if T < Enum %}
Which expanded to:
> 422 | when .== Class then new Class
> 423 |
> 424 | when .== GC:Module then new GC:Module
^
Error: unexpected token: Module (expecting ',', ';' or '
')
After doing that, no other command works until I quit and restart.
Overall I think this will be an amazing update, but I think it'll need just a bit more fine tuning before we merge it in.
Ping me if you have any questions, or need anymore testing done. Thanks for your work on this!
Features
__
or__previous_result
TODO:
Samples:
Status: In progress