cli: parse_opts should not actually spin off a repl

[richo]

While consuming pry programmatically in an internal project, I was surprised to discover that parse_options actually starts a pry session unless you mangle some internal state (clearing option_processors).

This patch breaks some APIs that I have no idea if there are meant to be guarantees around, so I can refactor if that's a problem, but in general causes parse_options to return the parsed options, which can then be passed to start.

[kyrylo]

Hi, @richo!

It would be nice if you could mention which APIs it breaks.

From local testing I have noticed a few oddities with this change:

• I noticed that pry -e 123 produces an unwanted newline in history (that is, execute pry -e 123 and press ↑)
• when you define Pry.config.exec_string in your ~/.pryrc, you can no longer use the -e switch as the config value takes precedence (this is a general Pry issue not related to your patch)
• #add_option_processor is unused now

Although I haven't tested the patch thoroughly, I remember that this part of code isn't tested well and if I recall correctly, there are some gotchas with respect to it. /cc @banister

Maybe, if you explained the problem you stumbled upon, we could probably find a different solution, because currently the motivation behind the change is a bit unclear to me.

[richo]

Sorry I missed this!

So the issue specifically is that calling Pry::CLI.parse_options actually drops you into a repl, and we wanted to embed pry (with option parsing) into an internal project.

I'm very happy to fix the newline bug, I think it should be fairly straightforward. I can also remove add_option_processor.

The only API it breaks, is the behaviour that previously Pry::CLI.parse_options dropped you into a repl and returned.. something. Which I think is probably not correct, but was definitely what it did before and I broke that. parse_options now parses options and returns them, which I think is outwardly more reasonable.

ping @kyrylo because I missed this for a couple of weeks to make sure you see this

[richo]

Just having had another read, it appears that add_option_processor constituted a part of the public API. I have no idea how to determine if anyone is using it, but my intuition is to leave it alone, unless you'd prefer that I don't.

[kyrylo]

Yeah, good catch. This is how it looked like previously (4 years ago): https://github.com/pry/pry/blob/d438a2f0370bfa99d7914cf06bacd8d6572484fa/lib/pry/cli.rb

And this commit introduced the change: https://github.com/pry/pry/blob/6a799a7fcd198e4bb4205dde055fca346176b867/lib/pry/cli.rb

So basically, you want to revert the feature :)

Although I don't know why it was made so exactly, my logic for keeping things as they are is that it was probably made on purpose. Only @banister knows the real reason. So I see 2 ways of resolving this issue.

1. @banister comments on this and he decides what to do.
2. I close this issue, because we don't want to break anything yet :)

[richo]

So I'd really like to do something about this sooner rather than later. We currently have a thing internally that looks like:

def self.start_shell!
  require 'pry'
  # This can be delurked once https://github.com/pry/pry/pull/1393 lands
  Pry::CLI.option_processors.clear

  Pry::CLI.parse_options
  NameSpace.pry(:quiet => true)
end

In order to respect ordinary pry options, and then drop into a repl attached to an internal object. I can add another method to do just the stuff we want, but the API will officially be confusing by that point.

Regardless, I'd rather not close the issue, even if the PR doesn't land because I think we can agree this is a bug in pry? eg, it violates the principle of least surprise pretty drastically.

[kyrylo]

The code you posted reveals the issue better now. I agree that it's a problem. I think @banister overlooked it. Sorry, I had a hard time remembering this code, because I haven't worked on it for over a year or so.

I'd be happy to merge this once the exec_string stuff is resolved.

[richo]

Cool, that's really helpful. I just fixed the newline bug, I'll have a quick skim over the precedence bug.

[richo]

Ok, and now the config files are loaded before the CLI options are parsed. This yields the weird (maybe?) behavior that they're additive but it's not really clear to me what the intended behaviour is here, and it does ~what multiple -es does on the CLI so I'm going to punt it over the fence to you, please let me know if you think it should be different.

r? @kyrylo closes #670

[kyrylo]

Thanks, good stuff! ✌️

[kyrylo]

Heya! I just discovered that this PR breaks the -r switch.

For example:

code/pry/repl[d70f065...]% pry -rrss      
[1] pry(main)> RSS
NameError: uninitialized constant RSS

Do you have time to investigate this? :)

[richo]

I can have a look, although it definitely seems to work in our environment. Poking now.

[richo]

Ah, it's an order of operations bug. I'll get you a PR today.

[richo]

#1435 makes the bug go away, and seems fairly idiomatic to me