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

[WIP] catching Signal::INT #41

Closed
wants to merge 9 commits into from

Conversation

jwoertink
Copy link
Collaborator

@jwoertink jwoertink commented Feb 9, 2017

This PR isn't ready yet, but as an initial start I wanted to get your thoughts and input on some stuff.

Before this, ^C and ^D were caught by the same process_input which would just exit the console. Now, ^D will exit (just like IRB), but ^C is caught. The next step will be to create a state, and manage the state of the input. Then using ^C to reset the state to the last known clean state.

Related: #33

@jwoertink
Copy link
Collaborator Author

Ok, update on this. I'm now setting a state on each command added to the command stack. The idea is when catching ^C, I can look at the command stack, and pop off the end all commands that are in a err state.

The issue I'm running in to now is that unexpected_eof errors don't push to the command stack immediately. So for these ones, I don't know their state. If I push these to the stack, then a lot of other weird things start to happen. I'll keep plugging away.

…o parse control characters. Need to figure out the event loop issue
@greyblake
Copy link
Member

greyblake commented Feb 14, 2017

ICR stucks when I am pressing Ctrl+C after opening a class:

icr(0.20.5) > class A
icr(0.20.5) >   ^C
icr(0.20.5) >   ^C
icr(0.20.5) >   exit
icr(0.20.5) >   quit
icr(0.20.5) >   ^C^Z
[5]+  Stopped      ---- Ctrl+Z

Copy link
Member

@greyblake greyblake left a comment

Choose a reason for hiding this comment

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

Sorry for delay. I left there couple of comments.

It feels like to implement this feature we need to introduce new entity, something like ComposedCommand or something. But I am not sure that I want to dig to deep into it right now.

I hope sooner or later Crystal community will have a proper REPL without side effects)

# returns `self`. Removes all values from the end of @commands where the state is `:err`.
def reset!
new_stack = commands.dup
commands.reverse.each { |c| c.state == "err" ? new_stack.pop : break }
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of this method? I looks like your intention is to pop last command if it was erroneous (maybe I miss something), cause with first non-erroneous command break ends iteration.
If so, why not to use simple if condition?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea behind this method is a "rollback". Each line you enter is like a commit. When you press ^C, it should rollback to the last good state. We need to keep the order in which things were entered, so I imagine the __exec_icr__ function would look something like...

1 + 1 # state is ok
a = 4 # state is ok
class Test # state is not ok
ebd  # state is not ok

then ^C is pressed and it looks like this.

1 + 1 # state is ok
a = 4 # state is ok

@@ -51,7 +59,7 @@ module Icr
process_command(new_command)
when :error
# Give it the second try, validate the command in scope of entire file
@command_stack.push(command)
@command_stack.push(command + " ##{Icr::DELIMITER}err")
Copy link
Member

Choose a reason for hiding this comment

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

Patching command this way, seems a little bit strange to me. I am afraid it can cause some underdesired side effects. There must be only 1 delimiter and result output.

Copy link
Collaborator Author

@jwoertink jwoertink Feb 14, 2017

Choose a reason for hiding this comment

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

Yeah, I thought about this too. I wanted to use a delimiter to make it easy just in case someone randomly enters a comment in to ICR. I figured using the built in one would be ok. Maybe we could have a secondary delimiter?

@jwoertink
Copy link
Collaborator Author

I get the same result. It's something to do with the Readline library that's causing that. If I switch to using a simple gets, it's fine. But then we run in to other issues like not being able to use arrow keys and such. I asked in the crystal gittr channel, and the best info I got was something with the Crystal event loop. I have to dig more in to it.

@jwoertink
Copy link
Collaborator Author

Just wanted to update on this (I haven't ditched it). I found a bug in crystal. Or, at least I'm thinking it's a bug....

I think there's a lot more Readline can do for us in this to make things a lot easier, but Crystal is missing a lot of what the readline lib has.

@veelenga
Copy link
Member

veelenga commented Oct 9, 2017

I would prefer not to implement/merge this. ICR currently is full of tricks and glitches. And this PR only complicates things.

@jwoertink
Copy link
Collaborator Author

Yeah, I agree. Crystal needs a bit more added to the Readline library to do this properly. I'll close this out for now, and we can touch back on it later.

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.

3 participants