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

brew irb improvements #14892

Merged
merged 4 commits into from Mar 8, 2023
Merged

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Mar 5, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

This resolves #14832.

It adds the two things mentioned in the original issue.

  1. 11a0ea1 The ability to pass --debug and --quiet to IRB without it choking.
/u/l/Homebrew (irb-improvements|) $ brew irb --debug
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`
irb(main):001:0> Context.current
=> #<Context::ContextStruct:0x00007fc8229a6220 @debug=true, @quiet=false, @verbose=false>
irb(main):002:0> exit
/u/l/Homebrew (irb-improvements|) $ brew irb --quiet
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`
irb(main):001:0> Context.current
=> #<Context::ContextStruct:0x00007f8a6da71b98 @debug=false, @quiet=true, @verbose=false>
irb(main):002:0> exit
/u/l/Homebrew (irb-improvements|) $ brew irb --verbose
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`
irb(main):001:0> Context.current
=> #<Context::ContextStruct:0x00007f80bca9e388 @debug=false, @quiet=false, @verbose=true>
irb(main):002:0> exit
  1. 6ab6d7c Local IRB history for REPL sessions.

I just added a IRB config file that gets loaded automatically if you set the IRBRC env variable with the path to it. It enables history which gets written to .irb_history in the repository directory and autocompletion. This apparently doesn't work with system Ruby for some reason but does work with portable Ruby which is better than nothing.

The only interesting thing here is that the file has a non-standard IRB config name to not clash with other IRB config files. Otherwise it would default to the closest .irbrc-like file which meant if you were in the brew repo directory it would get picked up instead of your global config.

References:

  1. 9f7ab25 Improves the Pry config

This separates the brew pry config from the global one and adds local history. Before this was just defaulting the global config and history files.

@BrewTestBot
Copy link
Member

Review period will end on 2023-03-07 at 00:00:00 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 5, 2023
The --debug and --quiet options weren't working before
with the REPL because IRB didn't recognize them.
@nandahkrishna nandahkrishna changed the title Irb improvements brew irb improvements Mar 5, 2023
@nandahkrishna
Copy link
Member

nandahkrishna commented Mar 5, 2023

I'm facing a weird issue while testing this PR -- the autocompletion works but there are some issues with the history:

  • Each time I spin up brew irb the history file is overwritten. So it stores the history for just one session. Is this intentional?
  • I'm not able to use the up arrow key upon starting a new brew irb session to use commands from the previous session that are stored in history.

Example usage:

➜ cat .irb_history
➜ brew irb
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`

WARNING: This version of ruby is included in macOS for compatibility with legacy software.
In future versions of macOS the ruby runtime will not be available by
default, and may require you to install an additional package.

irb(main):001:0> puts "Hi"
Hi
=> nil
irb(main):002:0> Form
Formatter                                FormulaOrCaskUnavailableError
Formula                                  FormulaOrCaskUnspecifiedError
FormulaAmbiguousPythonError              FormulaPin
FormulaCellarChecks                      Formulary
FormulaClassUnavailableError             FormulaSpecificationError
FormulaClassUnavailableErrorModule       FormulaUnavailableError
FormulaConflict                          FormulaUnknownPythonError
FormulaConflictError                     FormulaUnreadableError
FormulaInstallationAlreadyAttemptedError FormulaUnreadableErrorModule
FormulaInstaller                         FormulaUnspecifiedError
FormulaLock                              FormulaValidationError
irb(main):002:0> exit
➜ cat .irb_history
puts "Hi"
exit
➜ brew irb
==> Interactive Homebrew Shell
Example commands available with: `brew irb --examples`

WARNING: This version of ruby is included in macOS for compatibility with legacy software.
In future versions of macOS the ruby runtime will not be available by
default, and may require you to install an additional package.

irb(main):001:0> # up arrow doesn't work
=> nil
irb(main):002:0> # up arrow works within a session, upto the first command in that session
=> nil
irb(main):003:0> puts File.open(".irb_history").read
puts "Hi"
exit
=> nil
irb(main):004:0> exit
➜ cat .irb_history
# up arrow doesn't work
# up arrow works within a session, upto the first command in that session
puts File.open(".irb_history").read
exit
➜ # first session's history is gone
Here's my `brew config` if it helps.
➜ brew config
HOMEBREW_VERSION: 4.0.4-198-g6afaef3
ORIGIN: https://github.com/Homebrew/brew
HEAD: 6afaef3aaa27532525540102be9d4f20fe1a49b7
Last commit: 3 hours ago
Core tap origin: https://github.com/Homebrew/homebrew-core
Core tap HEAD: afafca6e56f5b73be3c8c224b4b8844236597a6c
Core tap last commit: 3 days ago
Core tap branch: master
Core tap JSON: 05 Mar 20:55 UTC
HOMEBREW_PREFIX: /usr/local
HOMEBREW_BAT: set
HOMEBREW_BAT_CONFIG_PATH: /Users/nanda/.batconfig
HOMEBREW_CASK_OPTS: []
HOMEBREW_DISPLAY: /private/tmp/com.apple.launchd.VPkNrITBRZ/org.xquartz:0
HOMEBREW_EDITOR: /usr/local/bin/nvim
HOMEBREW_GITHUB_API_TOKEN: set
HOMEBREW_GITHUB_PACKAGES_TOKEN: set
HOMEBREW_GITHUB_PACKAGES_USER: nandahkrishna
HOMEBREW_MAKE_JOBS: 12
Homebrew Ruby: 2.6.8 => /System/Library/Frameworks/Ruby.framework/Versions/2.6/usr/bin/ruby
CPU: dodeca-core 64-bit kabylake
Clang: 14.0.0 build 1400
Git: 2.37.0 => /Library/Developer/CommandLineTools/usr/bin/git
Curl: 7.79.1 => /usr/bin/curl
macOS: 12.6-x86_64
CLT: 14.0.0.0.1.1661618636
Xcode: N/A

.irb_config Outdated Show resolved Hide resolved
@apainintheneck
Copy link
Contributor Author

apainintheneck commented Mar 6, 2023

@nandahkrishna Thanks for taking a look at it.

No it's not supposed to work like that at all. I just tested it out on a newer Mac and got the same result which is unfortunate.

What's interesting is that is seems to be writing to the file just fine while simultaneously failing to load from it.

Edit: I wonder if this is related to system Ruby on MacOS.

Edit 2: It works with portable Ruby but not system Ruby.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Mar 6, 2023
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Mar 6, 2023
@BrewTestBot
Copy link
Member

BrewTestBot commented Mar 6, 2023

Review period ended.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks good so far, a few comments!

.irb_config Outdated

require 'irb/completion'

irb_history = (HOMEBREW_REPOSITORY/".irb_history")
Copy link
Member

Choose a reason for hiding this comment

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

Feels a bit weird having this in the repository rather than $HOME? Is this because the sandbox will let you write in the prior and not the latter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we want the history to be exclusive to Homebrew, I think it makes sense to put it in that directory. I don't think it's a good idea or expected behavior to interact with the global IRB history though if that's what you were suggesting.

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck Feels weird to put something user-specific like this in the repository. If there's no technical reason against it being e.g. $HOME/.brew_irb_history: I think that'd be preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It works that way too. Updated in deabd4a

.irb_config Outdated
@@ -0,0 +1,11 @@
# Not that we use a non-standard config file name to reduce
Copy link
Member

Choose a reason for hiding this comment

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

How about .brew_irbc or similar? Similarly, putting this under Library/Homebrew as a not dot file will make it a bit more discoverable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it made sense to make it a dot file since it's just a config thing but I have no preference here.

Copy link
Member

Choose a reason for hiding this comment

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

@apainintheneck I guess my thinking is that most editors etc. will hide it and we don't need to keep this naming if we're passing it explicitly.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly using brew_irbrc makes clearer it's for brew irb

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in deabd4a

@apainintheneck
Copy link
Contributor Author

I'm about ready to give up on getting this working with system Ruby. For whatever reason it seems to fail on two different system Ruby versions on different Macs and I don't know why. I still think this is useful to have though either way.

It does work with portable Ruby (and with installed Rubies as well) so if we could force that then everything would work perfectly but I doubt we want to do that. @Bo98, would that even be possible here? If we did that, in theory we could also ship a newer version of IRB with more bells and whistles.

Another thing that I was thinking about was saving pry history as well.

@apainintheneck
Copy link
Contributor Author

I found this old thread from 6 years ago which suggests it might be a problem with the Readline that gets built with system Ruby. The way they describe the issue sounds eerily similar to what I've seen when trying to debug it.

@apainintheneck apainintheneck removed the critical Critical change which should be shipped as soon as possible. label Mar 7, 2023
This enables history for `brew irb` sessions.
It saves that history to the repository directory.
The idea here is that the pry session history
should be separate for homebrew than the global
pry history.

We also ignore any .pryrc files so that they
don't interfere with this config.
.irb_config Outdated Show resolved Hide resolved
.irb_config Outdated Show resolved Hide resolved
@@ -63,9 +64,25 @@ def irb

ohai "Interactive Homebrew Shell", "Example commands available with: `brew irb --examples`"
if args.pry?
Pry.config.should_load_rc = false # skip loading .pryrc
Copy link
Member

Choose a reason for hiding this comment

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

This may be unexpected behaviour. May want to check with any @Homebrew/brew folks who use pry

Now both REPL history is written to $HOME.
- Pry: $HOME/.brew_pry_history
- IRB: $HOME/.brew_irb_history

The IRB config file has also been moved to the
library directory.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work here @apainintheneck, thanks for addressing all my comments. As you've had no responses to the pry stuff and the person I know uses pry has definitely seen it: let's ship this and be prepared to adjust if there's complaints.

Thanks again!

@MikeMcQuaid MikeMcQuaid merged commit e5c0fb4 into Homebrew:master Mar 8, 2023
23 checks passed
@apainintheneck apainintheneck deleted the irb-improvements branch March 24, 2023 03:25
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

brew irb improvements
4 participants