added coverage of multiple ids in tags#553
Conversation
|
|
||
| ``` | ||
| [ <covered-artifact-type> -> <specification-object-id> ] | ||
| [ <covered-artifact-type> -> <list-of-specification-object-ids> ] |
There was a problem hiding this comment.
| [ <covered-artifact-type> -> <list-of-specification-object-ids> ] | |
| [ <covered-artifact-type> -> <specification-object-id>] | |
| If a piece of code covers multiple specification items, you can also list them separated by comma. | |
| [ <covered-artifact-type> -> <specification-object-id>, <specification-object-id>] |
There was a problem hiding this comment.
Thanks @Davidius86 for the PR. I am started reviewing it. I will update main to 2.6.0 and create the release letter in another PR, so that we have a place where we can mention the change.
There was a problem hiding this comment.
Completeness: Please also add a short example of the syntax to .agents/skills/openfasttrace/SKILL.md.
There was a problem hiding this comment.
Completeness: Please add an short entry for this in the change log too (we are currently working towards 4.6.0).
| ``` | ||
|
|
||
| ##### Forwarding Requirements | ||
| ##### Needed Coverage |
There was a problem hiding this comment.
Good find. 👍
Compliance: I am guessing you are using an LLM to help with the work. Please note that according to our contribution guideline, LLM-assisted PR must state that.
There was a problem hiding this comment.
Completeness: Please also add tests that the features with requiring coverage and partial spec item definition still work in combination with the comma-separated covered IDs.
|
|
||
| private static List<String> parseCommaSeparatedList(final String input) | ||
| private static List<SpecificationItemId> parseCoveredIds(final String input) | ||
| { |
There was a problem hiding this comment.
Safety: Add null handling, please.
| final int counter) | ||
| { | ||
| final String uniqueName = this.file.getPath() + lineNumber + counter + coveredId; | ||
| final String CoveredIdStringsJoinedWithHyphen = coveredIds.stream() |
There was a problem hiding this comment.
UX: This is where the compactness gets to be a problem. While it is nice to be able to cover multiple spec ids in one line, the generated single ID from the code gets excessively long. I would suggest to instead create one "virtual" covering spec item id per covered id instead of a really long one that won't be readable.
There was a problem hiding this comment.
I'm not sure whether I got this correctly.
Does "virtual" mean that multiple new specification ids are generated, one per covered id?
That would create one entry in the trace report for each covered id of this tag.
It would also require to cover each of the specification ids derived from the covered ids.
I have only a limited knowledge on the use cases but I think I would be surprised if a tag would create more than one specification id. Especially if this behavior changes depending on whether a name has been specified or is auto generated.
You have a valid point here, the readability of the concatenated name is definitely low. The following options came to my mind:
- Use only the name of the first covered id (hides information / may be misinterpret)
- Use only a hash without any naming (decreases readability)
- Use an abbreviation (decreases readability)
Sorry, for this lengthy post. Please tell me which solution you prefer and I will adapt the PR accordingly.
There was a problem hiding this comment.
In a way a conceptual issue surfaces here. Usually requirements span a more or less tree-like structure that branches out from abstract to concrete. Where a top-level feature is the most abstract and the implementation the most concrete.
So it is rare that one code place covers a lot of higher-level requirements. At least if you write short methods and keep the coverage tags in the smallest possible scope.
So normally a method should cover only a single design requirements. In rare cases two or three, but never more.
This PR adds syntactic sugar that makes the tag more compact. But it is limited for example by newlines because then the simple parser won't work anymore. Especially, since you cannot predict what the comment characters are that need to come after line breaks.
All that is to say that the best solution here is to generate one item per covered ID. The only non-confusing exception would be if instead of just the covered artifact type there was the full tag ID. But we don't want to over-complicate things.
Let me make an example to make this more concrete:
// [impl -> dsn~assign-admin-privilege~1, dsn~log-security-event~3]
public void assignAdminPrivilege(final User user)Should become
impl~assign-admin-privilege-1b2f456~0→dsn~assign-admin-privilege~1impl~log-security-event-9876a3~0→dsn~log-security-event~3
There was a problem hiding this comment.
And
# [dsn -> req~assign-admin-privilege~1, req~log-security-event~3 >> impl, utest]
@startuml
Should become
dsn~assign-admin-privilege-1b2f456~0→req~assign-admin-privilege~1(needs:impl,utest)impl~log-security-event-9876a3~0→dsn~log-security-event~3(needs:impl,utest)
Which will probably automatically work, but needs a test at least.
There was a problem hiding this comment.
Sure, will do.
Side note: Thank you for sharing this perspective; it gave me a much clearer understanding of the problem we're facing. I do think, though, that in practice requirements often arise from different viewpoints (safety, security, user acceptance, technical limitations, architectural constraints, ... ) instead of following a purely tree-like structure.
2 participants
This PR enhances tags to define multiple covered ids in a tag.
ChatGPT 5.4 has been used for the first draft of the implementation and for the test.
The test was in almost in a good condition. The implementation has been heavily refactored after generation.
Documentation is written manually.