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

Dict completions in REPL #17017

Merged
merged 1 commit into from
Jul 5, 2016
Merged

Dict completions in REPL #17017

merged 1 commit into from
Jul 5, 2016

Conversation

bjarthur
Copy link
Contributor

i have found rapture with this hack. the REPL now detects if you're within the square brackets of a Dict, and tab completes based on the keys. it works for all key types, so long as you type the repr of it.

@@ -381,12 +381,44 @@ function bslash_completions(string, pos)
return (false, (String[], 0:-1, false))
end

function dict_identifier_key(str,tag)
if tag == :string
Copy link
Contributor

@tkelman tkelman Jun 19, 2016

Choose a reason for hiding this comment

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

wrong indent, should also use ===

@bjarthur bjarthur force-pushed the dictcompletions branch 3 times, most recently from 97e7a94 to 54d5f49 Compare June 19, 2016 15:16
@nalimilan
Copy link
Member

Looks nice! Any chance this could work for any Associative type? It would be great to get this for DataFrames column names.

@bjarthur
Copy link
Contributor Author

now works with all Associative types, but (DataFrame <: Associative) == false. to get the column names one uses names, not keys. not sure how to put this in Base if using DataFrames is required.

@quinnj
Copy link
Member

quinnj commented Jun 19, 2016

typeof(df) <: Associative is pry what you want

@nalimilan
Copy link
Member

Yes, I know DataFrame isn't an Associative currently, but since that's OT I didn't mention it. It would make sense to change this, since data frames really behave like a dictionary of column names to vectors (though they are also much more than that, and e.g. also act like matrices). Anyway, good to know it works with any type!

@ivarne
Copy link
Member

ivarne commented Jun 20, 2016

It seems somewhat unlikely to be a problem, but how long will this hang if somoene have a really large dictionary? Should there be a timeout?

You should probably also look into how errors in keys, collect, and repr will show up for a user, with bugs in their Associative implementation. Is there anything sensible we can do in that case?

if identifier != nothing
matches = []
for key in eval(Main,:(collect(keys($identifier))))
startswith(repr(key),partial_key) && push!(matches,repr(key))
Copy link
Member

Choose a reason for hiding this comment

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

Is the call to collect really needed, and should we use a temporary variable to avoid calling repr(key) twice?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we have generally tried to stay away from repl features that
can execute arbitrary code while you are editing. Could we avoid that by
supporting just a reasonable subset of key types?
On 20 Jun 2016 09:36, "Ivar Nesje" notifications@github.com wrote:

In base/REPLCompletions.jl
#17017 (comment):

function completions(string, pos)
# First parse everything up to the current position
partial = string[1:pos]
inc_tag = Base.syntax_deprecation_warnings(false) do
Base.incomplete_tag(parse(partial, raise=false))
end
+

  • if completing a key in a Dict

  • identifier, partial_key, loc = dict_identifier_key(partial,inc_tag)
  • if identifier != nothing
  •    matches = []
    
  •    for key in eval(Main,:(collect(keys($identifier))))
    
  •        startswith(repr(key),partial_key) && push!(matches,repr(key))
    

Is the call to collect really needed, and should we use a temporary
variable to avoid calling repr(key) twice?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/JuliaLang/julia/pull/17017/files/17da9f809389e84c028c0404833323031740e7be#r67646639,
or mute the thread
https://github.com/notifications/unsubscribe/ABjTf9KSHjxCXeSCCqlCKunOGuGnIK6Rks5qNkMMgaJpZM4I5Ln-
.

@bjarthur
Copy link
Contributor Author

collect and duplicate repr removed.

happy to add a timeout. can someone suggest how to go about doing that?

what specifically is the problem with using eval in the REPL? the utility and simplicity of being able to handle all key types is huge.

@ivarne
Copy link
Member

ivarne commented Jun 20, 2016

Timeout (based on time) should probably be implemented at another level in the REPL. I don't think that should be a requirement for this PR. The simple (partial) solution is to add a threshold (eg only look at the 1000 first keys), to limit the worst case runtime in case of big dictionaries.

I would strongly consider restricting the key type to Symbol, String (or AbstractString), and potentially Numbers? Other types are usually not used as Dict keys, and probably don't have syntax that makes tab completion meaningfull. Allowing other key types them might lead to hangs in the REPL,

PS: repr is probably much slower than you expect for strings, but I opened #17026 for that side discussion.

@bjarthur
Copy link
Contributor Author

i tested REPL responsiveness with Dicts of size up to 1e6. see code below. a bit slow but tolerable at 1e6. 1e5 was no problem at all.

foo=Dict(); for i=1:1000000; foo[i]=randstring();  end

it would be straightforward to have a limit beyond which tab completion was simply disabled. but does anyone actually ever use Dicts this big?

@quinnj
Copy link
Member

quinnj commented Jun 22, 2016

Did you test various key types/lengths? It'd probably be good performance of huge strings keys as well. I think it'd be good to put some kind of length cutoff.

@ivarne
Copy link
Member

ivarne commented Jun 22, 2016

but does anyone actually ever use Dicts this big?

I don't know, but if they do, a warning is better than a hang.

PS: your code should probably read foo[randstirng()]=i, to test lookup with strings, but I would not think there is a huge difference.

@bjarthur
Copy link
Contributor Author

1e6 strings of length 32 is also tolerable.

@bjarthur
Copy link
Contributor Author

code now includes a 1e6 limit on Dict completions.

partial_key = str[begin_of_key:end]
main_id = eval(Main,identifier)
typeof(main_id) <: Associative && length(main_id)<1e6 || return (identifier, nothing, nothing)
identifier, partial_key, begin_of_key
Copy link
Member

Choose a reason for hiding this comment

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

Should this return main_id so you only have to call eval once? (See the call for keys($identifier) below.)

@timholy
Copy link
Member

timholy commented Jun 30, 2016

Just got a vote for this functionality, so I searched and found this PR. Nice work, @bjarthur. Aside from the issue I commented on, this seems merge-worthy to me. Any objections?

@bjarthur
Copy link
Contributor Author

duplicate eval removed

begin_of_key = findnext(x->!in(x,whitespace_chars), str, end_of_indentifier+2)
begin_of_key==0 && return (identifier, nothing, nothing)
partial_key = str[begin_of_key:end]
main_id = eval(Main,identifier)
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be getfield?

@bjarthur
Copy link
Contributor Author

bjarthur commented Jul 1, 2016

last eval replace with getfield. i think everyone should be happy now.

@ivarne ivarne added the REPL Julia's REPL (Read Eval Print Loop) label Jul 4, 2016
@ivarne
Copy link
Member

ivarne commented Jul 4, 2016

I think this looks good, and is usefull functionality.

I found this minor nit while testing completions of string keys:

julia> a = Dict("1234"=>1)
Dict{String,Int64} with 1 entry:
  "1234"=>1

julia> a["1{\tab}"]
#turns into
julia> a["1234"]"]
# but I would expect
julia> a["1234"]

PS: I'm still somewhat sceptical that this attempts to work for all key types (via repr) instead of a limited predfined key types that we know we can handle safely. I can't figure out how to break it though, so unless others agree, this is good to go.

@bjarthur
Copy link
Contributor Author

bjarthur commented Jul 5, 2016

i don't agree that this should be the expected behavior. if we're using repr, then keys of type string include the enclosing double-quote characters. by typing the first and last characters of the key (the double quotes), you are asking that the completion mechanism match a prefix and a suffix. no completion mechanism i have ever come across works this way.

as a counter example, what would you expect in the following case:

julia> b = Dict(1234=>1)
Dict{Int64,Int64} with 1 entry:
  1234 => 1

julia> b[1{\tab}4]

@ivarne
Copy link
Member

ivarne commented Jul 5, 2016

if we're using repr, then keys of type string include the enclosing double-quote characters

Yea, I guess it is the repr thing I don't really like.

Anyway, the osx travis failiure seems unrelated and this can easily be reverted if it causes problems, so I'm going to merge now.

@ivarne ivarne merged commit a019a8f into JuliaLang:master Jul 5, 2016
mfasi pushed a commit to mfasi/julia that referenced this pull request Sep 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants