Depends on D3585
Add logic for storing the log data in the database.
Details
Details
cd services yarn run-backup-service
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 133 ↗ | (On Diff #11032) | error |
Comment Actions
Please make the summary more descriptive. Add the rest of SendLog functionality is a really general description of what is happening here. And a lot is happening here - is there a way to split this diff? (It's also confusing what do you mean by the rest - it doesn't sound like adding new todos should be a part of a diff which adds the rest).
Add logic to make SendLog functionality fully work.
A diff like that should definitely have a concrete test plan - it looks like at this point the logic should be easy to test
Comment Actions
Ok, I split the diff into multiple diffs. I hope it's now easier for you to review, I was trying to add a simple piece of code in every diff. The test plan you'd probably want to see is in D3651
Comment Actions
Thanks! It's a lot better!
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 58 ↗ | (On Diff #11178) | In this diff it isn't obvious why do we need to store value in this instead of receiving it via function argument |
| 76 ↗ | (On Diff #11178) | This diff shouldn't touch this function. I think we can keep this call - even if the function is empty |
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 112 ↗ | (On Diff #11219) | Let's revert also this change |
| 58 ↗ | (On Diff #11178) | Ok, that makes sense. Generally, there's nothing wrong in adding something and not using it, but it can cause some confusion. The solution is to catch such issues before putting up a diff and explaining in the summary or inline comments why something is added. |
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 55–69 ↗ | (On Diff #11219) | Still don't understand why we are putting implementations in .h files like this. I know @karol-bisztyga created a task to track this, but I don't understand why we ever landed code with this anti-pattern. I would like us to prioritize removing all of the implementations from .h files wherever possible |
| services/backup/docker-server/contents/server/src/Reactors/server/SendLogReactor.h | ||
|---|---|---|
| 55–69 ↗ | (On Diff #11219) | I don't see a point in doing it now (in this diff) if there's a task for this. I'm going to do this task soon, so please do not worry. |
You don't need to use React.useMemo(...) here.
The useMemo() hook is helpful for maintaining referential equality so that objects will be considered "shallowly equal" (== in JS) and we can avoid re-renders. This is helpful for objects (including Map(), Set(), etc), arrays (which are objects), and functions (which are objects).
On the other hand, strings in JS are considered shallowly equal if they have the same contents, so we don't have to worry about re-renders if the "content" stays the same.
See below:
(In like C++, which you've been working w/ recently, std::strings can be allocated on the heap (unlike integers, booleans, etc) which may have been part of your reasoning that shallow equality would check reference instead of contents?)