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

RFC: add and and or suggestions to UndefVarError, per #37469 #40545

Merged
merged 1 commit into from
May 27, 2021

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Apr 20, 2021

Replaces #37471 and closes #37469. In the future, I envision this list becoming much longer, with completions based upon context and spell-checking (either here, or perhaps somewhere like OhMyRepl). But testing this out first to see whether others agree this is a good approach.

julia> or
ERROR: UndefVarError: or not defined
suggestion: Use `||` for short-circuiting boolean OR.

julia> and
ERROR: UndefVarError: and not defined
suggestion: Use `&&` for short-circuiting boolean AND.

Co-authored-by: Jerry Ling (@Moelf)

@vtjnash vtjnash added REPL Julia's REPL (Read Eval Print Loop) error messages Better, more actionable error messages labels Apr 20, 2021
@oscardssmith
Copy link
Member

Should this also reference & and |?

@@ -811,4 +811,13 @@ function shell_completions(string, pos)
return Completion[], 0:-1, false
end

Base.Experimental.register_error_hint(UndefVarError) do io, ex
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to put this into the REPL stdlib, whereas e.g. the noncallable number hint is in base? Either seems fine, but I think it would be good to be consistent about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I want it to be unavailable if REPL isn't loaded, since that lets us do more advanced things here (though I should perhaps move it to __init__)

Copy link
Member

Choose a reason for hiding this comment

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

Should we move the noncallable number one here as well then?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not entirely sure. On the one hand, we want the default experience to have good errors, even on CI. On the other hand, we don't want to encumber the runtime with complicated error messages that aren't needed, even on CI.

Copy link
Member

Choose a reason for hiding this comment

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

Since these hint are really only intended for beginners, I would lean towards having them defined by the REPL. Users contributing to packages and looking at CI probably don't benefit much from them.

@vtjnash
Copy link
Member Author

vtjnash commented May 4, 2021

@oscardssmith I think there's many possible additions. The goal for this PR was to merge a minimal set, then let others help improve the list.

@vtjnash vtjnash added the triage This should be discussed on a triage call label May 27, 2021
@oscardssmith oscardssmith merged commit c509eff into master May 27, 2021
@oscardssmith oscardssmith deleted the jn/37471 branch May 27, 2021 18:30
@oscardssmith
Copy link
Member

triage approves.

@oscardssmith oscardssmith removed the triage This should be discussed on a triage call label May 27, 2021
@simeonschaub
Copy link
Member

Was ist discussed whether this should live in the REPL stdlib or in Base?

@JeffBezanson
Copy link
Member

I guess this is ok but it doesn't work for infix:

julia> 1 and 2
ERROR: syntax: extra token "and" after end of expression
Stacktrace:
 [1] top-level scope
   @ none:1

I think the reason to put this in the repl is that we'd eventually like all interactive, convenient kinds of stuff to be there. This one is a small amount of code, but the full set of error printing logic is pretty complex, and shouldn't be mandatory for all programs. Until then though, the inconsistency may not be worth it.

shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
Co-authored-by: Jerry Ling <[email protected]>

Co-authored-by: Jerry Ling <[email protected]>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Co-authored-by: Jerry Ling <[email protected]>

Co-authored-by: Jerry Ling <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
error messages Better, more actionable error messages REPL Julia's REPL (Read Eval Print Loop)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add docstrings for and and or
4 participants