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

Preserver previous command results (avoid rerunning commands) #128

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

owen2345
Copy link

@owen2345 owen2345 commented Apr 13, 2021

Features

  • feat: support to print previous command result with __ or __previous_result
  • feat: Preserve regular command results as serialized values and make them available to be used by next commands (avoid re running commands)
  • feat: Scan declared variables inside a command to make available in next commands

TODO:

  • Add tests
  • Calculate all variables declared inside the command (currently only takes the first variable using regex)

Samples:

  $> 10 # => 10
  $> puts "previous result: #{__}" # => previous result: 10

  $> var1 = "Var1" # => ""
  $> puts "var1 is: #{var1}" # => var1 is: Var1

  # Array support
  $> arr = [1,2,3]
  $> puts "arr result: #{arr} and size: #{arr.size}" # => arr result: [1, 2, 3] and size: 3

  # Hash support
  $> hash = { s: "String", number: 10 }
  $> puts "result: #{hash} and number: #{hash["number"]}" # => result: {s: "String", number: 10} and number: 10

  #complex objects (Amber app)
  $> require "./config/application.cr"
  $> qty_articles = Article.count
  $> article = Article.create(title: "Article 1")
  $> puts "qty_articles is #{qty_articles} vs #{Article.count}" # qty_articles is 0 vs 1
  $> puts "last article is: #{article.title}" # last article is: Article 1

Status: In progress

@jwoertink
Copy link
Collaborator

Well this is interesting! I'll come back to review a bit later, but my first question is what is the difference between _p and __ (two underscores we currently use)?

@owen2345
Copy link
Author

Well difference between _p and __ is that __ can not be used as a variable in the next command, against _p which can be used as a normal variable like:

puts "previous value: #{_p}" 

Btw: I tried to replace _p into __ but unfortunately crystal does not accept as a valid variable name, either _;

@jwoertink
Copy link
Collaborator

__ can not be used as a variable in the next command

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 _p, then we should remove __ with an explanation as to why _p is a better solution. Hope that makes sense.

@owen2345
Copy link
Author

@jwoertink you were right! Sorry my mistake. I was too tired yesterday then I could not test it very well.

@jwoertink
Copy link
Collaborator

No worries! I just want to make sure we're getting this right. Thanks for contributing ❤️

Copy link
Collaborator

@jwoertink jwoertink left a 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!

@jwoertink jwoertink mentioned this pull request Sep 25, 2021
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.

None yet

2 participants