-
Notifications
You must be signed in to change notification settings - Fork 120
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
Add copy command #1044
Add copy command #1044
Conversation
565421b
to
7affd25
Compare
lib/irb/command/copy.rb
Outdated
HELP | ||
|
||
def execute(arg) | ||
output = irb_context.workspace.binding.eval(arg) |
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 think we need to apply the output inspection method here so the copied output will be closer to what the users may expect.
For example, the output
here by default will be the to_s
result of the evaluation result, but IRB's default output inspector pretty prints the result inspect.
Additionally, we need to make sure colorizing is NOT applied to the copied 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.
The problem with using the pretty printer is that colorizing output is hardcoded into it, and I don't think we want it in our clipboard.
Unless there's a pretty printer that I've missed, the options are:
- Grab colorized output, strip color before copying. Least invasive, and will respect the currently configured inspector.
- Install a new pretty printer analogous to the default color printer that does basically the same thing but doesn't colorize output. Don't like this one because we're conflating two concepts and hardcoding a command to use a specific kind of inspection method.
- Like above,
color_printer.rb
to accept a parameter that turns colorizing on / off. Most invasive.
I like 1, but will defer.
7affd25
to
8819e5e
Compare
8819e5e
to
1b409ba
Compare
lib/irb/command/copy.rb
Outdated
end | ||
|
||
def clipboard_program | ||
@clipboard_program = "pbcopy" if executable?("pbcopy") |
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.
Should we also have return if defined?(@clipboard_program)
so we don't need to reevaluate the presence of the executables?
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 updated it to be @clipboard_program ||=
, but it looks like the command is re-instantiated each time so it doesn't matter. I could cache the value elsewhere (say, the class), but that has a subtle bug where changes to IRB.conf[:CLIPBOARD_PROGRAM]
won't reflect once this has been computed.
914962e
to
590af2b
Compare
ENV['IRB_COPY_COMMAND'] = '' | ||
IRB.setup(__FILE__) | ||
assert_equal('', IRB.conf[:COPY_COMMAND]) |
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.
Unsure what the behavior here should be. Setting this env var overrides the other clipboard programs we use, so right now this would cause copy
to stop working.
However, we could just as easily interpret ''
to mean "fall back to pbcopy
".
.It Ev IRB_COPY_COMMAND | ||
Overrides the default program used to interface with the system clipboard. |
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.
README.md
Outdated
@@ -358,6 +358,7 @@ irb(main):002> a.first. # Completes Integer methods | |||
- `NO_COLOR`: Assigning a value to it disables IRB's colorization. | |||
- `IRB_USE_AUTOCOMPLETE`: Setting it to `false` disables IRB's autocompletion. | |||
- `IRB_COMPLETOR`: Configures IRB's auto-completion behavior, allowing settings for either `regexp` or `type`. | |||
- `IRB_COPY_COMMAND`: Overrides the default program used to interface with the system clipboard. |
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.
Does the documentation need to be updated anywhere other than here and the man page?
0f721f1
to
f0faf98
Compare
lib/irb/command/copy.rb
Outdated
end | ||
rescue StandardError => e | ||
warn "Error: #{e}" | ||
warn "Is IRB.conf[:COPY_COMMAND] set to a bad 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.
Currently, copy typo
will show this COPY_COMMAND warning message.
I think this message should be placed inside def copy_to_clipboard
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.
Updated! Here's how it works now:
λ irb → λ git sin.add-copy-command → ./bin/console
irb(main):001> copy typo
Error: undefined local variable or method `typo' for main
irb(main):002> copy 5
Copied to system clipboard
irb(main):003> IRB.conf[:COPY_COMMAND] = 'lulz'
=> "lulz"
irb(main):004> copy 5
No such file or directory - lulz
Is IRB.conf[:COPY_COMMAND] set to a bad value?
irb(main):005>
f0faf98
to
f308c9e
Compare
Closes ruby#753
f308c9e
to
b4eb6be
Compare
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.
This is great, thank you ❤️
Closes #753