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

Qualify public, unexported bindings in REPL help #52524

Merged
merged 4 commits into from Feb 8, 2024

Conversation

ararslan
Copy link
Member

Fixes #52472, which was caused by names being changed to also return public, unexported symbols in #50105. Note that this restores previous behavior. A case could be made to instead add the public, unexported bindings as suggestions with the appropriate qualification.

Not entirely sure how to test this so I'd welcome any suggestions.

@ararslan ararslan added stdlib:REPL Julia's REPL (Read Eval Print Loop) kind:bugfix This change fixes an existing bug labels Dec 14, 2023
@jishnub
Copy link
Contributor

jishnub commented Dec 14, 2023

I'm not sure how hard it'll be, but suggesting public but unexported names will be really helpful. This may be a separate PR though

@ararslan
Copy link
Member Author

I had a go at it here in a separate commit.

@ararslan
Copy link
Member Author

Example from #52472 (screenshot rather than code copy-paste to show that highlighting is still working as expected):
Screenshot 2023-12-14 at 1 14 24 PM

@testset "search prioritizes exported names" begin
# Prioritize exported/local names if they exactly match
lines = helplines("dango")
@test startswith(lines[1], "search: dango TestSuggestPublic.dango dingo")
Copy link
Member Author

Choose a reason for hiding this comment

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

Interestingly, this test fails on some platforms and works on others. 🤔 It works for me locally on macOS ARM but fails on macOS ARM CI. It also fails on Linux x86_64 but works on macOS and FreeBSD x86_64. I have not the slightest idea why that could happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vtjnash, any idea why this might be different across platforms?

Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

SGTM

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 1, 2024

Looks like the sort order seems arbitrary, since the first 2 have the same name?

Error in testset REPL:
Test Failed at /Users/julia/.julia/scratchspaces/a66863c6-20e8-4ff4-8a62-49f30b1f605e/agent-cache/default-honeycrisp-XG3Q6T6R70.0/build/default-honeycrisp-XG3Q6T6R70-0/julialang/julia-master/julia-e479b4cf1d/share/julia/stdlib/v1.11/REPL/test/docview.jl:166
  Expression: startswith(lines[1], "search: dango TestSuggestPublic.dango dingo")
   Evaluated: startswith("search: TestSuggestPublic.dango dango dingo range", "search: dango TestSuggestPublic.dango dingo")

@ararslan
Copy link
Member Author

ararslan commented Feb 2, 2024

Part of the implementation aims to disambiguate by adjusting the edit distance so that one case is always sorted before the other but perhaps it was not implemented correctly.

@LilithHafner LilithHafner changed the title Don't suggest inaccessible bindings in REPL help Qualify public, unexported bindings in REPL help Feb 3, 2024
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 6, 2024

I am not sure where range came from in the suggestion list, but is it possible these have the same score for TestSuggestPublic.dango and dango?

@ararslan
Copy link
Member Author

ararslan commented Feb 7, 2024

Certainly possible, though if they did then I would have expected these tests to fail: https://github.com/JuliaLang/julia/pull/52524/files#diff-c16f754f85cbf6a46764e56fb6572adf5e13692d753d561d21df8a07b14b8b39R60-R65

stdlib/REPL/src/docview.jl Outdated Show resolved Hide resolved
stdlib/REPL/src/docview.jl Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Feb 7, 2024
@inkydragon
Copy link
Sponsor Member

Only ":windows: build x86_64-w64-mingw32" failed: (git error)

fatal: index-pack failed
⚠️ Warning: Checkout failed! exit status 0xc000013a (Attempt 1/3 Retrying in -4.6340586s)

LGTM?

@KristofferC KristofferC merged commit 95df060 into master Feb 8, 2024
5 of 8 checks passed
@KristofferC KristofferC deleted the aa/repl-help-52472 branch February 8, 2024 16:15
@inkydragon inkydragon removed the status:merge me PR is reviewed. Merge when all tests are passing label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bugfix This change fixes an existing bug stdlib:REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Puzzling namespace-less help message for an unexported binding
5 participants