-
Notifications
You must be signed in to change notification settings - Fork 604
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
Simplify Prompt API #1877
Simplify Prompt API #1877
Conversation
anyway, i hope you can consider the other improvements in that MR in this one, which you ignored completely so far.. including attaching a |
#1860 branch: it looks easier to read but not only that, a Hash is not an object we can extend or implement behaviour on (such as a custom inspect). imho to represent a prompt as a concept (not just a hash with some key/value pairs) it should be represented with a class or a Struct. |
another advantage to using a Struct: we can use |
This branch breaks the old API, while #1860 did not. ➜ pry git:(prompt-simpler-api) ✗ pry
[1] (pry) main: 0> _pry_.prompt = [proc{}, proc{}]
=> [#<Proc:0x00007f9a3a831108@(pry):1>, #<Proc:0x00007f9a3a8310e0@(pry):1>]
Traceback (most recent call last):
... |
i thought this MR was opened because mine "changed an internal API", but this PR breaks a public API that has existed since the earliest days of Pry instead. |
Another possible advantage of using a class/struct, that's not possible with a Hash since it is not extensible/possible to implement behaviour for: def waiting_proc
value[0]
end
def incomplete_expression_proc
value[1]
end the names could be better, it's just another example of an advantage not using a Hash has here. |
4ee5665
to
b07fa3e
Compare
I understand your points, they are valid. This PR focuses on hiding
That's true and it's on purpose. Users are encouraged to transition to the new API. Perhaps, it's wiser to deprecate it gracefully. That said, the old way will be unsupported eventually because now we have public API. I should've deprecated this in v0.12.0, really. The extendability point makes sense but I don't think we need it right now, do we? We will be able to change how it works internally without a problem if we find the need. |
yeah, at least for one release i suggest adding something like: if Array === new_prompt
_pry_.output.puts "A two-proc array as a prompt is deprecated, please migrate to Pry::Prompt"
# support new_prompt as usual beyond here.
end i think it is possible users have wrote custom prompts in their
Fair enough, but this hash (as it is now) is returned from public API methods: it is not a big change in terms of diff to do that (see my last PR), but if you want to do it in another PR thats cool. |
i'm also fine if you don't want to do it at all, but i think it would be an improved API and only a useful feature in the future - it gives a lot more options than returning a Hash does, which seems like a really bad object to use as a representation of a prompt. |
one last suggestion, it can be separate from the above: add a |
891e5b9
to
0f6a8cb
Compare
I deprecated the old API. It'll emit a warning now:
Please check. |
looks good, i see you have added doc strings like |
0dc10b6
to
de5de83
Compare
"The default Pry prompt. Includes information about the current expression \n" \ | ||
"number, evaluation context, and nesting level, plus a reminder that you're \n" \ | ||
'using Pry.' do |context, nesting, _pry_, sep| | ||
# @return [String] |
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.
# @return [String] the name of a prompt
.
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.
Didn't add on purpose because it's duplicative and obvious :)
# @return [String] | ||
attr_reader :name | ||
|
||
# @return [String] |
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.
@return [String] the description of a prompt
de5de83
to
da5ab88
Compare
Good feedback. Thanks 👍 |
looks pretty good, nice work. |
@@ -19,7 +19,7 @@ class Pry | |||
# # In [4]: | |||
# @since v0.11.0 | |||
# @api public | |||
module Prompt | |||
class Prompt |
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 you used a Struct here you wouldn't break API compatibility, since it also supports access through #[]
.
def initialize(name, description, prompt_procs) | ||
@name = name | ||
@description = description | ||
@prompt_procs = prompt_procs |
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.
@prompt_procs
is a better name than @value
for sure, but why break API compatibility? we could easily stay backwards compatible if we did two things:
class Prompt < Struct.new(:name, :description, :value)
- this would allow#[]
to continue to work, soPry::Prompt[:simple][:value]
,Pry::Prompt[:simple][:name]
, etc would still work.
- use alias_method to allow
value
to be called, print a deprecation message.
we're going in the direction where when next pry is released, running pry
will cause an error in .pryrc
and die.
edit: it won't die since pry rescues errors in .pryrc and prints a short backtrace, but still not a good ux
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.
maybe we could just do this, not sure:
def [](key)
if %w(name description).include?(key.to_s)
warn "A prompt is now an instance of `Pry::Prompt` and not a Hash. \n" \
"Please use `prompt.#{key}` instead."
public_send(key)
elsif key.to_s == 'value'
warn "A prompt is now an instance of Pry::Prompt and not a Hash. \n" \
"Please use `prompt.prompt_procs` instead."
@prompt_procs
else
super # NoMethodError
end
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.
Didn't mean to break compatibility, it's just an oversight. Good catch. I added this method.
end | ||
|
||
# @return [Proc] the proc which builds the wait prompt (`>`) | ||
def wait_proc |
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.
great
|
||
# @return [Proc] the proc which builds the prompt when in the middle of an | ||
# expression such as open method, etc. (`*`) | ||
def incomplete_proc |
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.
great
@@ -112,15 +138,15 @@ def prompt_name(name) | |||
end | |||
|
|||
add( | |||
'simple', | |||
:simple, |
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.
why do you insist on using a Symbol for this argument, it is actually just converted back to a string when being stored in a Hash. is it an appearance thing? because one could argue all the other arguments are strings, so this one should be too so we're uniform :P
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.
That's a Ruby convention to have hash keys as symbols. Prompt[:name]
is hash-like so it feels more natural to me. Internally it's indeed string but symbols look nicer as public API IMO.
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 don't know if that's a ruby convention, maybe a Rails one. np anyway, not an issue here.
Before this change when you set a prompt, you have to do the following: ```rb Pry.config.prompt = Pry::Prompt[:simple][:value] ``` The `[:value]` part was leaking implementation details and it proved to be an unnecessary step. With this change we can do the following: ```rb Pry.config.prompt = Pry::Prompt[:simple] ``` `[:value]` is omitted. I have also refactored some tests and removed irrelevant ones. The Array API for prompt is deprecated: `Pry.config.prompt = [proc {}, proc {}]` emits a warning now.
448acda
to
bc26e40
Compare
Good feedback @R-obert 👍 With help of it we improved Prompt a ton. I'm going to merge this since the PR is quite large and it's hard to manage it already. I'm happy to address any extra feedback in further PRs. |
@kyrylo |
Before this change when you set a prompt, you have to do the following:
The
[:value]
part was leaking implementation details and it proved to be anunnecessary step.
With this change we can do the following:
[:value]
is omitted.I have also refactored some tests and removed irrelevant ones.
The Array API for prompt is deprecated:
Pry.config.prompt = [proc {}, proc {}]
emits a warning now.