-
Notifications
You must be signed in to change notification settings - Fork 61
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 a better error. #1846
add a better error. #1846
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks! One small suggestion on what we can print.
arbor/cable_cell_param.cpp
Outdated
[&] (const auto& p) { | ||
using T = std::decay_t<decltype(p)>; | ||
if constexpr (std::is_same_v<init_membrane_potential, T>) { | ||
os << "init-membrane-potential"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have been printing everything in the decor part of the code as s-expressions, so maybe it's worth sticking to that style? Also we could print the value while we're at it.
Something like (init-membrane-potential -65)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I deliberated on the same question, here's what I swayed me against
- telling what might be enough information
- ...and not overloading the reader, the line is quite long already.
- also, where to draw the line?
(density ('name' (...)))
? Or print all the parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough. Should we keep it out of the public header as it's not printing the "full" information about the class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good point to think about. It just prints the outer DSL constructor in s-exp-speak.
At some point we had opened an issue on a 'general object printing facility', maybe it's
a good time to take that a bit seriously?
For now, I'll change <<
to print_paintable
and hide it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent.
Since we have support for ACC loading the errors thrown by
paint
are no longersufficiently informative. Even before that it was already hard to correlate source
and locus of painting errors.
Example
This adds more details to the error message