Conversation
|
|
||
| #### Specification Item | ||
| `req~specification-item~2` | ||
| `req~specification-item~4` |
There was a problem hiding this comment.
- You didn't update the content of this item.
- If you plan to update it: please only increment the revision by one
- If you don't plan to update it: please don't increment the revision
This is an internal refactoring, so updating the spec is not necessary in my view.
|
|
||
| #### Specification Item | ||
| `dsn~specification-item~3` | ||
| `dsn~specification-item~4` |
There was a problem hiding this comment.
You didn't update the text, see my comment in doc/spec/system_requirements.md
| coveringItem.getItem() | ||
| .toBuilder() | ||
| .addCoveredId(getId()) | ||
| .build(); |
There was a problem hiding this comment.
This won't solve the issue. Calling build() will just create a new instance, but you don't do anything with the new instance. To update the coveringItem you need to replace it with the new instance.
There was a problem hiding this comment.
Thanks for your contribution @bkalika . But the problem lies a lot deeper and needs more refactoring. I have that on my todo list.
The short version is that we need to distinguish between items and links to avoid mutable objects. With links we will have a append only list. I will fix that at some point, but it's not my priority right now.
@kaklakariada, I think we should close this PR and have an issue ticket for the refactoring instead.
Also, this is not a feature PR.
There was a problem hiding this comment.
@redcatbear, thanks for the clarification! That makes sense. I understand the deeper refactoring should separate specification items from links and make links append-only. If you already have a design in mind for the bigger refactoring, I’m happy to leave it to you. but if you’d like, I can also try to take a first pass at it now while I already have the code and the problem in context.
github-project-automation
Bot
moved this from 📫 Backlog
to 🔨 In Progress
in OpenFastTrace
3 participants
Closed #572