Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
iox-#590 Remove error handler calls from hoofs #1027
iox-#590 Remove error handler calls from hoofs #1027
Changes from 12 commits
8fd2cf4
a697f00
39824c1
cb1e916
c7ed437
7e3f82d
cb654f2
873613c
f3d922b
91454e4
1d21571
69d9448
28823ae
0c581ae
fd0b941
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Even though I suggested
ErrorHandlerMock
and I introduce aLoggerMock
, I'm not fully convinced by this name. This is not a request to change it, because currently I can't suggest a better name but if you or another reviewer can find one I would highly appreciate it ... and while writing this,TestingErrorHandler
/testing::ErrorHandler
came to my mind. What do you think, is this better or worse?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 was thinking about this also back and forth. My conclusion was: The
ErrorHandlerMock
is not really a mock more of aErrorHandlerHook
. I kept the name as the termMock
will trigger a "Hey this is only for tests"-thought when developer read it. That's why I think we should both keepLoggerMock
andErrorHandlerMock
. Other names could be misleading.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.
Yeah, I'm a bit torn on this but like I said, I don't have an idea where I would say it is much better than the current naming.
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.
If it has full functionality and is an error handler used in tests it should not be called a
Mock
of any kind. I thinkTestingErrorHandler
orTestErrorHandler
is better for this reason.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.
Except that
std::function
is gone this does not look and feel better (naming with Mock, template arg) to use. We can fix this later of course but really need to think about how this could be used in a nice way.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.
Done in #1028
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.
This is hard. Furthermore #1028 states the following:
Separate errors of posh and C binding
and as it seems currently the include guards, include headers, cmake files are not yet adjusted there. Please create a separate PR for this the other one is already huge.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.
On the 2nd thought, I think what we need to do is separate the
ErrorHandler
fromerror_handling.hpp
.iceoryx_hoofs
'serror_handling.hpp
will have only one entryEXPECTS_ENSURES_FAILED
. So in the end:error_handler.hpp
(hoofs)error_handling.hpp
(hoofs)error_handling.hpp
(posh)error_handling.hpp
(C binding)Added to #1099
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 can't recall the decision on the
k
prefix. Did we decide to remove it? If yes, I guess it will be done in a follow up PR together with the removal oferrorCallback
?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.
There wasn't a decision on that topic yet. If we will change it, I would for sure do it in a follow-up PR.
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.
Okay, please keep in mind to let us come to a decision before the callback is removed from the
errorHandler
since it would fit quite nice with such a PRThere 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.
@elBoberido @mossmaurice I think we introduced the
k
prefix so that it conforms to the autosar guidelines and then we found out that this is actually pretty nice since otherwise we would run into some trouble with our macro.So whoever does this - be prepared for fun :D
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.
@elfenpiff that's no problem. One has just to replace
#define CREATE_ICEORYX_ERROR_ENUM(name) k##name,
with#define CREATE_ICEORYX_ERROR_ENUM(name) name,
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.
The removal of the 2nd arg will be done either in #1028 or a follow-up.
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.
Maybe we should create small separate issues for the error handler since I am already a little bit lost. And you have several comments where you state, will be done in this PR or will be done in another follow up or in this issue and so on. And when I look at those PRs, issues etc. either the task is done incompletely or nothing is mentioned in the issue to this particular task.
For me as a reviewer it is hard to track all of this.
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'll close this PR and will open an new one based on #1099
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.
Not sure if this is a good idea since it might clash with
ERROR_NAMES
from posh once the posh errors are moved there. What was the reason to move it out of the class scope?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.
Have a look at #1028. The names will be unique for each module e.g.
C_BINDING_ERROR_NAMES[]
. Hence no clashes are possible.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.
Okay, so it will be taken care of when the split actually happens
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 think we have to do some restructuring. Issue #590 states nothing about the error handling, we have several hints that certain tasks are performed in several PRs or in this issue but this is nowhere really tracked or mentioned.
Why is the error handler part of #590 ? (I know why but this is hard to keep track when this is nowhere mentioned)
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.
That's true. I'll close this PR here and re-open it as part of a new issue.