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

More descriptive error for environment variable not found #6042

Closed
wants to merge 1 commit into from

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Mar 4, 2014

When ERROR: key not found: "foo" happens deep in some internal function, it's not always obvious that it's referring to an environment variable. Bikeshedding on the error message welcome (or if this needs to be done differently), but I think this is a special enough case that it should get a specific message.

@tknopp
Copy link
Contributor

tknopp commented Mar 4, 2014

While the error message is cleaner, its now not possible anymore to catch KeyErrors (or are they mapped to the same exception?). IMHO error should be used in a lot less situations. More specific exceptions make the exception system a lot more useful.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 4, 2014

Thanks, that's the kind of thing I meant by "if this needs to be done differently." I don't know enough about exception handling to be sure how to do this properly. Would throw(KeyError("environment variable not found: $k")) work here?

@ivarne
Copy link
Member

ivarne commented Mar 4, 2014

That will look funny when it prints like

ERROR: key not found: "environment variable not found: foo"

@tknopp
Copy link
Contributor

tknopp commented Mar 4, 2014

Actually I am not an expert on Julia exceptions either but have worked a lot with exceptions in C++ and C#. And from this I know that it is pretty important to
a) have an exceptions hierarchie
b) and actually also use it
c) and don't (!!!) allow to throw integers ;-)

@tknopp
Copy link
Contributor

tknopp commented Mar 4, 2014

@ivarne: Are the exceptions defined in the C part of Julia? Maybe KeyError should only print key not found when no string is passed as constructor argument.

@ivarne
Copy link
Member

ivarne commented Mar 4, 2014

Some exceptions is defined in C because of bootstrapping issues. The rest is defined in base/boot.jl. The printing of the exceptions is defined in base/repl.jl as methods on the showerror function.

@tkelman tkelman mentioned this pull request Mar 4, 2014
@tknopp
Copy link
Contributor

tknopp commented Mar 4, 2014

Okay thanks. I find it a little bit confusing that the naming Exception and Error seems to be a little mixed. In most cases one cannot know if it is an Error or an Exception at the point where it is thrown. In the case of the KeyError it can be part of the regular workflow to catch it if an environement variable is not defined.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 4, 2014

That will look funny when it prints like

ERROR: key not found: "environment variable not found: foo"

Agree that it looks funny. Maybe remove the duplication of "not found," just say throw(KeyError("environment variable $k"))?

@JeffBezanson
Copy link
Member

I think the argument to KeyError should be the key itself in all cases.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 5, 2014

I think the argument to KeyError should be the key itself in all cases.

That's perfectly reasonable. What about adding a subtype of KeyError called EnvError or something, for which the showerror method says "environment variable not found" instead of "key not found"?

@ivarne
Copy link
Member

ivarne commented Mar 5, 2014

If we had inheritance from concrete types, that would be the right approach. Currently the Julia exception system is not designed for the inheritance hierarchy, we know (and love) from Java and Python.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 7, 2014

I'll close this PR for now, until I figure out a better way to do what I'm going for.

It seems like making an almost identical copy of KeyError specifically for environment variables that only differs in the error message might be the solution, but I don't know how many places that would need to touch.

@ivarne I don't particularly like anything about either of those languages but that's beside the point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants