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

Print full paths in spellchecking suggestions when needed for disambiguation #2109

Merged
merged 3 commits into from Nov 5, 2018

Conversation

Projects
None yet
4 participants
@nojb
Copy link
Contributor

commented Oct 18, 2018

See MPR#7864. It's a bit hackish, so suggestions welcome!

Before:
image

After:
image

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

I'm a bit disturbed by the proposed behavior: at the point in the program where the hint is given, foo and A.foo mean the same thing, right? (In fact foo refers to "the previous foo binding" but the user cannot access this.)

When I saw the bugreport, my idea for a fix was to simply accumulate a set of names (removing duplicates in the process), but I had no time to look at this.

@alainfrisch

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

foo and A.foo mean the same thing, right?

Not necessarily:

let foo = 0
module X = struct let foo = 1 end
module Y = struct let foo = 2 end
open X
open Y
let _ = fooo

gives:

Hint: Did you mean foo, foo or foo?

Still, I wouldn't be shocked to suggest only foo (once).

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

I agree with @gasche. Initially I had been playing with an example as in @alainfrisch's note above, which is why this felt logical to me; but on second thought I agree it does not seem the right thing to do in general. I will implement the simpler alternative of simply eliminating duplicates.

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 18, 2018

( @alainfrisch I didn't immediately understand your point, but you mean that the proposed implementation would say foo, X.foo or Y.foo, and that only two out of those three paths go to the same definition at the point of error.)

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch from f41594d to 3e0395d Oct 18, 2018

@@ -546,7 +546,8 @@ let spellcheck env name =
else if dist = best_dist then (head :: best_choice, dist)
else acc
in
fst (List.fold_left (compare name) ([], max_int) env)
fst (List.fold_left (compare name) ([], max_int)
(List.sort_uniq String.compare env))

This comment has been minimized.

Copy link
@Armael

Armael Nov 1, 2018

Member

Currently, the order of the elements in the output list is the reverse of the order in the env input list. After this patch, it is set to the reverse of the alphabetical order (as can be seen in the currently failing test).

I think it's fine for this PR to change the order (the env input list is coming from a fold on the environment and hence in an unspecified order anyway). I would just slightly prefer if it was the alphabetical order, not the reversed alphabetical order :-).

This comment has been minimized.

Copy link
@nojb

nojb Nov 1, 2018

Author Contributor

Thanks, fixed.

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch from 3e0395d to b4c68a2 Nov 1, 2018

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch from b4c68a2 to 9f1531e Nov 1, 2018

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch from fcf90aa to d79a2ee Nov 1, 2018

@Armael

This comment has been minimized.

Copy link
Member

commented Nov 1, 2018

Could you maybe add a test demonstrating the fix? The code that you show in the initial message, for example.

@Armael

Armael approved these changes Nov 1, 2018

Copy link
Member

left a comment

In any case, I'm approving the PR. Feel free to merge if you feel like adding the test is not worth the effort :-).

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2018

Thanks! I will add a test, but haven't found the time yet.

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch 2 times, most recently from 12078b1 to 82e400b Nov 5, 2018

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

Added a test. I didn't know where to put it (I didn't find any test dedicated to the spellchecking suggestion), so it ended up in typing-core-bugs...

@nojb nojb force-pushed the nojb:spellchecking_disambiguation branch from 82e400b to 1d6546a Nov 5, 2018

@Armael Armael merged commit 0fa81c7 into ocaml:trunk Nov 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Armael

This comment has been minimized.

Copy link
Member

commented Nov 5, 2018

Thanks!

@nojb nojb deleted the nojb:spellchecking_disambiguation branch Nov 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.