Details
- Reviewers
ashoat tomek - Commits
- rCOMM6da201fb0202: [services] Add documentation for base reactors
None - there are just comments
Diff Detail
- Repository
- rCOMM Comm
- Branch
- reactors-docs
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/lib/src/client-base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 51 | Don't like how we're copy-pasting code comments... it seems inevitable they will get out of sync. Is there some way to avoid this? Also curious for @palys-swm's perspective | |
| services/lib/src/client-base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 51 | If they do go out of sync, then they do for a reason. Each of them is individual, just the initial versions are the same. On the other hand, some things should stay the same. We could link the reader to someplace else with the mutual descriptions. Not sure what's the best way of handling this though. | |
| services/lib/src/client-base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 29–30 | Maybe it is a good idea to make it a virtual function of BaseReactor? | |
| 39 | Is there a way to avoid linking to a specific version? This might easily go out of sync. (or at least mention that the proper version of the class should be accessed). | |
| 45 | This is redundant, as the function is virtual. | |
| 51 | Having comments that need to be synchronized is a bad idea. But here I agree with what @karol-bisztyga said, at least for prepareRequest comment: Each of base reactors is a separate abstract class and establishes its own contract regarding this virtual function. Changing anything in the contract of one reactor doesn't affect other reactors, so the comments don't need to be synchronized in any way. They may look similar, but for me we should treat it as a coincidence. On the other hand, if there are comments that need to be repeated and kept in sync, they should come from a base class, just like I'm suggesting for the start function. | |
| services/lib/src/client-base-reactors/ClientWriteReactorBase.h | ||
| 11 | Why there is a TODO? | |
| services/lib/src/client-base-reactors/ClientBidiReactorBase.h | ||
|---|---|---|
| 29–30 | It doesn't appear in ServerBidiReactorBase and ServerReadReactorBase | |
| 39 | I did it on purpose. The goal is to link the methods in their state for a specific version. If we upgrade and if everything works, then ok but if something breaks with these methods, we'll change the code anyways (in this case we should also change this comment). Having a link to a specific version is important as without it, we cannot link a specific line in a certain file where these methods are exactly. We can always link the whole file as an alternative. | |
| 45 | You mean pure virtual. I know, I just wanted to explicitly say this, that in this method the reactor-specific logic is going to be placed. I can remove this. | |
| services/lib/src/client-base-reactors/ClientWriteReactorBase.h | ||
| 11 | leftover, thx | |