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

Add more specific exception message when conversion fails #68

Open
fatteneder opened this issue Jan 28, 2022 · 1 comment
Open

Add more specific exception message when conversion fails #68

fatteneder opened this issue Jan 28, 2022 · 1 comment
Labels
enhancement New feature or request

Comments

@fatteneder
Copy link
Contributor

Hi,

I might have a suggestions for an improvement of default error handling in terms of descriptiveness.

Example

@option struct MyType
   str::String = "abc"
end

dict_wrongkey = Dict("mystr"=>"abc")
from_dict(MyType, dict_wrongkey)

dict_wrongvaluetype = Dict("str"=>1)
from_dict(MyType, dict_wrongvaluetype)

The call from_dict(MyType, dict_wrongkey) will throw an InvalidKeyError with a descriptive message that mentions
the invalid key,

ERROR: InvalidKeyError: invalid key mystr, possible keys are: str

whereas the latter gives a MethodError where one cannot see which parameter of the dictionary failed the conversion,

ERROR: MethodError: Cannot `convert` an object of type Int64 to an object of type String

Workaround

A workaround for this would be to overload the from_dict method generically, e.g.

function Configurations.from_dict(::Type{OptionType}, ::OptionField{f_name}, ::Type{T}, x::S) where {OptionType, f_name, T, S}
  try
    return convert(T, x)
  catch e
    if e isa MethodError && e.f == convert
      error("Failed to convert '$x' to type '$T' for field '$f_name'")
    end
  end
end

However, this feels a little hacky, because I think the exception handling should be separated from the conversion method.

Question: Would you be interested in reviewing a merge request that implements a new error type similar to InvalidKeyError?

Cheers

@Roger-luo
Copy link
Owner

Yes, I like this idea, I agree the error handling is less intuitive, please go ahead to implement a new error type

@Roger-luo Roger-luo added the enhancement New feature or request label Jan 28, 2022
fatteneder added a commit to fatteneder/Configurations.jl that referenced this issue Jan 28, 2022
Roger-luo pushed a commit that referenced this issue Feb 10, 2022
* implement FieldTypeConversionError (#68)

* add rethrow of uncaught exception

* Insert _from_dict_errorhandled inplace of from_dict to catch FieldTypeConversionErrors

* merge _from_dict_errorhandled with from_dict

* fix test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants