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

give more information in StringIndexError #36054

Merged
merged 2 commits into from
Jun 5, 2020
Merged

Conversation

KristofferC
Copy link
Member

Before

julia> str = "abcdefghκijklmno"
"abcdefghκijklmno"

julia> str[10]
ERROR: StringIndexError("abcdefghκijklmno", 10)
Stacktrace:

Now

julia> str = "abcdefghκijklmno"
"abcdefghκijklmno"

julia> str[10]
ERROR: StringIndexError: invalid index [10], valid previous index [9] ('κ'), valid next index [11] ('i')

@KristofferC KristofferC added error messages Better, more actionable error messages strings "Strings!" labels May 28, 2020
@KristofferC
Copy link
Member Author

Bikeshed over valid previous index [1] ('α')?

Perhaps valid previous index [1]=='α'?

@@ -11,6 +11,15 @@ struct StringIndexError <: Exception
end
@noinline string_index_err(s::AbstractString, i::Integer) =
throw(StringIndexError(s, Int(i)))
function Base.showerror(io::IO, exc::StringIndexError)
s = exc.string
i = thisind(s, exc.index)
Copy link
Member

Choose a reason for hiding this comment

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

StringIndexError is exported and may be (mis)used by user code — maybe defensively check for exc.index < firstindex(s) && return invoke(showerror, Tuple{IO, Any}, io, exc) before doing something that may throw?

@StefanKarpinski
Copy link
Member

Bikeshed over valid previous index [1] ('α')?

I like that version fine, it's certainly clearer to me than [1]=='α'.

@StefanKarpinski
Copy link
Member

Oh but "previous valid index" reads more naturally than "valid previous index". There's some rule about how adjectives get sorted in English and "valid previous index" violates them (in some way that I only know intuitively, I have no idea what the rules actually are, just that they exist).

@StefanKarpinski
Copy link
Member

Same for "valid next index" which would be "next valid index" instead.

@mbauman
Copy link
Member

mbauman commented May 28, 2020

We use pairs to connect indices to values in some cases; it could be nearby valid indices 9=>'κ', 11=>'i'? Of course the irregular plural is annoying at the end of the string, but the previous and next aren't doing much heavy lifting since I can see that 9<10<11.

@KristofferC
Copy link
Member Author

Oh but "previous valid index" reads more naturally than "valid previous index".

Yeah, I thought about this too but decided on this to sort of emphasise the "valid nextind" and "valid prevind" connection but I can change the order if it is too weird.

@KristofferC
Copy link
Member Author

Did some tweaks based on comments

@JeffBezanson JeffBezanson merged commit 8c65f8d into master Jun 5, 2020
@JeffBezanson JeffBezanson deleted the kc/indexerror branch June 5, 2020 00:08
simeonschaub pushed a commit to simeonschaub/julia that referenced this pull request Aug 11, 2020
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 strings "Strings!"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants