Depends on D4252
I spotted some obvious bugs in this class. They've not been spotted before because the compilation doesn't see them. I guess it's because we do not yet use this class.
Details
Details
- Reviewers
tomek - Commits
- rCOMM81ca99963ae2: [services] Lib - fix bugs in ClientWriteReactorBase
None really. It doesn't affect the compilation, but I wanted to make these two right so if we ever use this class, it will be better.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I guess it's because we do not yet use this class.
More accurately, it's because the test plan for the diff that initially committed this class was insufficient.
When you put a diff up, you should have a plan to test that code. If you have code that you haven't had a chance to test yet, you should not be putting that code up for review. It's better to wait a bit before putting up a stack of diffs (until you have tested it) then you put up untested code, and later come back and revise it after you've tested.
By testing code you save your reviewers time, reduce churn, and make sure we aren't launching broken features.
Comment Actions
I understand.
Things we can do now:
- abandon this diff
- remove this class and write it again when it's needed
- land this
- land this and add a comment/failing assert saying that the code's not been tested
I'd land it. It is true this is not tested but if we ever need this class, a lot of work already will be done.
Comment Actions
I added an error that this class is not tested, if you feel like we should do something different here, please, let me know