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

[Feature] Return parameter name on get_parameter exception #868

Closed
maxlein opened this issue Sep 24, 2019 · 3 comments
Closed

[Feature] Return parameter name on get_parameter exception #868

maxlein opened this issue Sep 24, 2019 · 3 comments
Labels
enhancement New feature or request more-information-needed Further information is required

Comments

@maxlein
Copy link

maxlein commented Sep 24, 2019

Wouldn't it be a nice feature to return the missing parameter name instead of just throwing that some parameter is missing?

Something like this:

template<typename T>
   T get_parameter(const std::string & parameter_name)
   {
     auto error = fmt::format("Parameter {} not set!", parameter_name);
     return get_parameter_impl(
              parameter_name,
              std::function<T()>([&]() -> T {throw std::runtime_error( error.c_str() );}));
   }`
@mabelzhang mabelzhang added the enhancement New feature or request label Sep 26, 2019
@ivanpauno ivanpauno added the more-information-needed Further information is required label Sep 27, 2019
@ivanpauno
Copy link
Member

What method are you calling? Could you provide an example please?

It is always nice to have the clearer error message as possible, so adding the parameter name is a good idea.
What is not clear from your description is where that is missing.

If you find the place it is missing, consider sending a PR addressing this, thanks!

@maxlein
Copy link
Author

maxlein commented Sep 29, 2019

I am calling exactly the method shown above from parameter_client.hpp, just with the parameter name output added.

And as I am looking this up on the repo, I see it was already added to the source. #833
In my Dashing release its not included yet.

@ivanpauno
Copy link
Member

Closing, as it has been solved by #833.

If you want the fix in Dashing, please open a backport PR (i.e. a PR targeting dashing branch with 64c0f86 cherry-picked).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request more-information-needed Further information is required
Projects
None yet
Development

No branches or pull requests

3 participants