mob
mob copied to clipboard
remotemobprogramming
Uniform exit
Every error will result in an error message displayed and exiting mob with a status code of 1 (nonzero). This was not so previously. I switched all errors to warnings that didn't lead to a failed execution of mob.
Looks good to me. One thing I was thinking about if it would make sense to have a function error which gets a message, and an optional error which the does the printing of the error and automatically exiting the program with error code 1. What do you think about that?
That’s what I had in mind. Also, with an optional way to fix the error. Basicall all info we would send to the VS Code plugin
I see that you prefer exit(1) over returning an error. I also see the benefit in that it possibly makes the production code less bloated. But I also see the trade-off that concerns me. The problem is that it makes everything harder to test. An exit is particularly hard to test. In my experience, something that is hard to test is hard for a reason (a smell), and there is usually a better way to do it. The smell in my opinion is, that we have high level code that invokes exit. IMO this should not be the concern of high level code, but it should be the very outer layer that handles such a thing. The high level code/inner layers should just return an error, on which the outer most layer may react with invoking an exit. Then the tests, which don't even invoke the outer layer, don't care about the exit in the first place. Testing becomes simple again.
Totally agree with your assessment. The thing is the git calls in the code, of which we have a lot. They might fail and stop the program. Having explicit error handling for every git command would make the code really bloated and less readable. Every git call would need an if afterwards that checks the error and returns if there is an error. That’s basically why I chose exit over error, with all the downsides. I miss my exceptions in golang… Or did I miss something?
I understand. Seems like golang is just designed like this. I need to look into the "panic and recover" alternative https://www.bacancytechnology.com/blog/golang-error-handling
Take a look at this guide: https://golangbot.com/panic-and-recover/ I think it's well explained. Behaves similar to try/catch. I'm just not yet sure how this would affect testability.
Perhaps there's a middle ground here. We could have errors returned as part of the command methods (start, next, etc.) and when an error happens at a lower level, we use the same logic but invoke an exit explicitly at that time. This might make our code more testable while still preventing us of having a error handling if command for every git call we make.
Then you still have to exit and return after every git failure, and you need to not forget about the return in order to test it. And you need to overwrite exit everytime in the tests. Except if you do not test for the failures in the first place.
I'm thinking if you use panic instead, you don't have to add an arbitrary return. Maybe you could take advantage of the recover in the tests. You could exit in a recover in the prod code, too.
@hollesse @gregorriegler have a look at the latest commit. I played with panic and a return object.
No, always felt like 80% done, but never really great. That's why I was hestiant to merge, and now it deviated a little bit.
I need to have a look at this again. The goal of the issue is good, but I have no idea how we implemented it in the end. Need to take some time and think about it again