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

Reline.add_dialog_proc should not accept nil as proc #532

Merged
merged 1 commit into from
Apr 15, 2023

Conversation

tompng
Copy link
Member

@tompng tompng commented Apr 12, 2023

If you pass nil to add_dialog_proc, Reline suddenly raise error.

% ruby -rreline -e "Reline.add_dialog_proc(:a,nil);Reline.readmultiline('prompt>'){true}"
prompt>/usr/local/lib/ruby/3.3.0+0/reline/line_editor.rb:583:in `instance_exec': no block given (LocalJumpError)
	from /usr/local/lib/ruby/3.3.0+0/reline/line_editor.rb:583:in `call'
	from /usr/local/lib/ruby/3.3.0+0/reline/line_editor.rb:618:in `call'
        ...

I chage add_dialog_proc not to accept nil
I change to remove dialog_proc when nil is passed.
There was no way to remove dialog_proc before this pull request.

@st0012
Copy link
Member

st0012 commented Apr 12, 2023

Should we still take nil but make it as unsetting the previously set dialog?

  • If :a is set, Reline.add_dialog_proc(:a,nil) removes it from @dialog_proc_list
  • If :a is not set, Reline.add_dialog_proc(:a,nil) just does nothing (not adding :a to @dialog_proc_list, ofc)

@tompng
Copy link
Member Author

tompng commented Apr 12, 2023

take nil but make it as unsetting

Thanks! That looks better. I will do it.

I found a code in irb's test that expecting unsetting behavior.

https://github.com/ruby/irb/blob/c2874ec121d8d70b13ba89c2dbdde72961c2f0ce/test/irb/test_input_method.rb#L41-L43

def test_initialization_without_use_autocomplete
  original_show_doc_proc = Reline.dialog_proc(:show_doc)&.dialog_proc
  ...
ensure
  Reline.add_dialog_proc(:show_doc, original_show_doc_proc, Reline::DEFAULT_DIALOG_CONTEXT)
end

I think test was working because original_show_doc_proc was always not nil

@tompng tompng force-pushed the dialog_proc_nonnullable branch from fe3eeba to a2ed68a Compare April 13, 2023 11:28
@tompng tompng marked this pull request as draft April 13, 2023 11:31
@tompng tompng force-pushed the dialog_proc_nonnullable branch from a2ed68a to 4d3ccb0 Compare April 13, 2023 11:36
@tompng tompng marked this pull request as ready for review April 13, 2023 11:43
test/reline/test_reline.rb Outdated Show resolved Hide resolved
@tompng tompng force-pushed the dialog_proc_nonnullable branch from 4d3ccb0 to cc5eb8a Compare April 15, 2023 05:41
@st0012 st0012 added the bug Something isn't working label Apr 15, 2023
@tompng tompng merged commit 43283b2 into ruby:master Apr 15, 2023
@tompng tompng deleted the dialog_proc_nonnullable branch April 15, 2023 09:32
matzbot pushed a commit to ruby/ruby that referenced this pull request Apr 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants