-
Notifications
You must be signed in to change notification settings - Fork 118
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
Display show_cmds
's output in a pager when in TTY environment
#647
Conversation
fa5816a
to
025892d
Compare
lib/irb/cmd/show_cmds.rb
Outdated
@@ -1,6 +1,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
require "stringio" | |||
require "rdoc/ri/driver" |
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.
Do we need to wrap this with begin rescue
like irb is doing in completion?
Or this file is delayed loaded so we don't need to rescue it? (maybe the loading timing will change in the future?)
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.
The pager implementation part of rdic/ri/driver
is about 30 lines of 1500 lines.
Maybe we could implement it in IRB? then we can customize the command line option like
# https://github.com/ruby/rdoc/blob/5db4767dda4b4320eeac84aba152ab56ec20b839/lib/rdoc/ri/driver.rb#L1470
pagers = [ENV['RI_PAGER'], ENV['PAGER'], 'pager', 'less', 'more']
↓
# IRB's ansi format pager
pagers = [ENV['RI_PAGER'], ENV['PAGER'], 'pager --foobar', 'less --raw-control-chars', 'more --raw-control-chars']
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.
Do we need to wrap this with begin rescue like irb is doing in completion?
Since we've declared rdoc
as a dependency in #648, rdoc
should always be installed along with irb
. If that didn't happen for some reason, it's something else we need to fix.
Put it in another way: I think we can remove the rescue block in completion and other places.
@hsbt can you help me confirm that if my assumption above is correct? Or when irb
is used as a default gem, rdoc
could still be missing? (example)
Maybe we could implement it in IRB? then we can customize the command line option like
Generally I agree with this direction, but I found a few concerns when implementing it:
- Should
IRB::Pager
also acceptENV['RI_PAGER']
?- If it doesn't, users would get inconsistent behaviour where IRB's pager doesn't pickup their config for RI pager
- If it does, it sounds like a weird coupling with
rdoc
?
- Should
IRB::Pager
set different pager options (-r
for example) thanrdoc
's?- This will bring similar concerns the above question
I think if we want to avoid using rdoc
's pager, we should do that for all features so we can get rid off of the above issues. For example, IRB should only use rdoc
to get documentation content and handle the display itself. But at the same time the effort would grow significantly.
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.
Because it's necessary to pass -r
to less
in order to correctly display IRB's ANSI escape sequences, I eventually decided to implement IRB::Pager
with the support of ENV['RI_PAGER']
for now.
I think we can later replace the use of RDoc pager in show_doc
and doc dialog with something like this:
name = "String"
driver = RDoc::RI::Driver.new({})
found, klasses, includes, extends = driver.classes_and_includes_and_extends_for(name)
doc = driver.class_document(name, found, klasses, includes, extends)
formatter = RDoc::Markup::ToAnsi.new
contents = doc.accept(formatter)
IRB::Pager.page do |io|
io.puts contents
end
Can you describe your environment setup? I can't reproduce it on my machine with VS Code Terminal, iTerm2, or Terminal.app
Sorry I don't get this part. Do you mean you also need to set |
I think I'm using mac pre-installed less
and I can reproduce it with
|
That's weird because I'm using the same
And with it, these 3 examples all rendered correctly:
If I combine the 2 versions:
I get: |
It turns out because I use ohmyzsh, which sets |
20505ee
to
54a68ca
Compare
This can: - Make it easier to scroll up and down the commands list - Avoid pushing up users' previous output - Allow users to do basic search with `/<word>`
|
||
if pager.first == 'less' || pager.first == 'more' | ||
pager << '-R' unless pager.include?('-R') | ||
end |
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.
If the user has PAGER=less
set, we still want to make sure -R
or any future flags are appended to the command so the content could be displayed properly without asking users to change things.
|
||
class << self | ||
def page | ||
if STDIN.tty? && pager = setup_pager |
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.
I'm not sure if we should check STDIN.tty?
or STDOUT.tty?
. In most cases they should return the same value?
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.
👍 It's consistent with IRB shows message Switch to inspect mode.
when stdin is not a tty.
irb
→ both tty
irb > output_file
→ stdin is a tty
cat input_file | irb
→ stdout is a tty
cat input_file | irb > output_file
→ neither is tty
environment (ruby/irb#647) This can: - Make it easier to scroll up and down the commands list - Avoid pushing up users' previous output - Allow users to do basic search with `/<word>` ruby/irb@f94e8a66dd
This can:
/<word>
Demo
show_cmds-in-pager.mp4
Closes #646