Repository navigation
Fix #388 - #390
Fix #388#390antoineatstariongroup merged 4 commits into
Conversation
samatstariongroup
left a comment
samatstariongroup
left a comment
There was a problem hiding this comment.
Review feedback on the [Property] metadata added in this PR. The normative PIM model (ptc-25-02-29) is not in the repository, so the multiplicity points below are checked against Resources/ptc-25-02-30.json; please confirm them against the PIM.
1. [Property] multiplicities that disagree with the OpenAPI document (DTO and POCO)
These members are marked [1..1], while the OpenAPI document declares them nullable:
| Member | Files | Attribute | OpenAPI |
|---|---|---|---|
CommitReference.Deleted |
SysML2.NET/PIM/DTO/CommitReference.cs, SysML2.NET/PIM/POCO/CommitReference.cs |
[1..1] |
nullable |
Branch.Head |
SysML2.NET/PIM/DTO/Branch.cs, SysML2.NET/PIM/POCO/Branch.cs |
[1..1] |
nullable |
Record.Description |
SysML2.NET/PIM/Record.cs |
[1..1] |
nullable |
Query.Where |
SysML2.NET/PIM/DTO/Query.cs, SysML2.NET/PIM/POCO/Query.cs |
[1..1] |
nullable |
If the PIM agrees with the OpenAPI document, these should be lowerValue: 0.
In SysML2.NET/PIM/DTO/MergeResult.cs, MergeCommit is marked [0..1] but typed as a non-nullable Guid, so an absent merge commit cannot be represented. DataDifference uses Guid? for the same situation.
2. Derived flag differs between DTO and POCO
In SysML2.NET/PIM/POCO/Project.cs, Branches and Tags are marked isDerived: true. In SysML2.NET/PIM/DTO/Project.cs the same members are not. Both should carry the same value.
3. Read-only flag is applied inconsistently
ExternalData.Id and ProjectUsage.Id are marked isReadOnly: true, in both DTO and POCO. ExternalRelationship.Id is not, in either SysML2.NET/PIM/DTO/ExternalRelationship.cs or SysML2.NET/PIM/POCO/ExternalRelationship.cs.
4. Stub exception type
Commit.QueryVersionedData() and DataVersion.QueryProject() in SysML2.NET/PIM/POCO/ throw NotImplementedException, and this PR documents that in their <exception> tags. The repository rule is NotSupportedException for stubs, so please change both the throw and the XML doc.
| /// Gets or sets the commit to which the branch is currently pointing. It represents the latest state of the | ||
| /// <see cref="Project"/> on the given branch | ||
| /// </summary> | ||
| [Property(lowerValue: 1, upperValue: 1)] |
There was a problem hiding this comment.
the purpose of the Property C# attribute is to act as self documentation. My preference would be to write the whole propery, including repeating the default values. The intent is that a reader of the class can immediately see the related info coming from the UML model.
example:
[Property(xmiId: "abc", aggregation: AggregationKind.None, lowerValue: 1, upperValue: 1, isOrdered: false, isReadOnly: false, isDerived: false, isDerivedUnion: false, isUnique: true, defaultValue: null)]
|
Note that the comment about multiplicity has been ignored since it was based on the OpenAPI file, not based on the UML file (which defines the PIM) |
|
|
||
| using (Assert.EnterMultipleScope()) | ||
| { | ||
| Assert.That(mergeCommit.PreviousCommits, Is.EqualTo(MergeCommitPredecessors)); |
There was a problem hiding this comment.
the test checks PreviousCommits with Is.EqualTo, which depends on order, but the PR declares the property unordered. It should use Is.EquivalentTo
There was a problem hiding this comment.
hi, string interpolation inside the log statement is not the way to do it. should we not use the template of the logger and also check whether the log level is enabled?
There was a problem hiding this comment.
I'll update that, was part of the inital and hand-written implementation
|
Prerequisites
Description
Fix #388
Fix PIM inconsistencies