Item for UserPersist database entity
Depends on D3082
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h | ||
|---|---|---|
| 5 ↗ | (On Diff #9366) | We're not using this here. I can also see that the same happens in blob - we include items in DatabaseEntitiesTools. Is it for convenience, or we should avoid it? |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp | ||
| 30 ↗ | (On Diff #9366) | We're copying the vector, but I guess this might be a good idea - if we don't do that, someone could create this object, modify the vector variable used to create this and that would result in this object being changed. |
| 55–57 ↗ | (On Diff #9366) | Is it the recommended way of handling the bools? |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h | ||
| 22 ↗ | (On Diff #9366) | Why this isn't const? |
| services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h | ||
|---|---|---|
| 5 ↗ | (On Diff #9366) | I think this has been mentioned before. The idea for including all entities in this file is we can include only the DatabaseEntitiesTools.h file and then be able to use all the entities with no need to include a header for every entity in every file where we want to use them. I think this is a nice shortcut and it doesn't hurt. If you insist on having every entity mentioned explicitly where it's used, we can refactor this later. |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp | ||
| 30 ↗ | (On Diff #9366) | We probably could also move the vector. |
| 55–57 ↗ | (On Diff #9366) | I'm not sure if recommended. Basically, we can skip the database part here as it's transparent to the problem |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h | ||
| 22 ↗ | (On Diff #9366) | This can be const but I think we should refactor all services |
| services/backup/docker-server/contents/server/src/DatabaseEntities/DatabaseEntitiesTools.h | ||
|---|---|---|
| 5 ↗ | (On Diff #9366) | It shouldn't matter too much in our case, so I guess we can prefer the convenience here and keep it included in this header. |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp | ||
| 30 ↗ | (On Diff #9366) | I'm usually slightly afraid of breaking the objects that someone gives us... but not sure, maybe it's a good idea. |
| 55–57 ↗ | (On Diff #9366) | I guess it's ok - it doesn't matter too much. Have you checked if in that case we could use SetN / GetN? They should write / read int, so maybe it would be a little safer? |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h | ||
| 36 ↗ | (On Diff #9366) | We should include vector |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.cpp | ||
|---|---|---|
| 30 ↗ | (On Diff #9366) | Wasn't really sure what to do with this, so I created a task in the backlog as this doesn't seem as the highest prior right now: https://linear.app/comm/issue/ENG-724/think-about-passing-vectors-as-arguments Anyway, thanks for pointing this out! |
| 48 ↗ | (On Diff #9366) | |
| 55–57 ↗ | (On Diff #9366) | As a rule of thumb, I decided to always go with strings when dealing with dynamo DB. |
| services/backup/docker-server/contents/server/src/DatabaseEntities/LogItem.h | ||
| 36 ↗ | (On Diff #9366) | right |
Comment Actions
Discussed offline with @jimpo, removing him from the reviewer list. Jim if you happen to find some time before this gets landed, your review will be always welcome!
Comment Actions
Putting him back as a non-blocking reviewer (it won't change the diff state from accepted, but will put it on his queue)