Display UPP identifiers in the output. Little bit of optimising query #20
Conversation
concordances/cypher.go
Outdated
| NewModelIdentifiers neoIdentifier `json:"newModelIdentifiers"` | ||
| OldModelTypes []string `json:"oldModelTypes"` | ||
| OldModelIdentifiers neoIdentifier `json:"oldModelIdentifiers"` | ||
| UUID string `json:"UUID"` |
There was a problem hiding this comment.
Should this be "uuid" to be idiomatic json?
concordances/cypher.go
Outdated
| OldModelIdentifiers neoIdentifier `json:"oldModelIdentifiers"` | ||
| UUID string `json:"UUID"` | ||
| Types []string `json:"types"` | ||
| Identifiers neoIdentifier `json:"identifiers"` |
There was a problem hiding this comment.
Logically identifiers would represent multiple ids? Not just one neoIdentifier? If neoIdentifier as a struct represents more than one id, that should be called neoIdentifiers instead
There was a problem hiding this comment.
Yeah, agreed. Hangover from the previous model.
| RETURN canonical.prefUUID as prefUUID, p.uuid AS UUID, labels(p) AS oldModelTypes, labels(p) AS newModelTypes, | ||
| {labels:labels(ids), value:ids.value} AS oldModelIdentifiers, | ||
| {labels:labels(leafId), value:leafId.value} AS newModelIdentifiers | ||
| MATCH (p)-[:IDENTIFIES]-(ids:Identifier) |
There was a problem hiding this comment.
Is this no longer required to be a Concept?
There was a problem hiding this comment.
The Concept constraint is defined in the first line of the query, so it's not needed again.
| @@ -208,6 +195,10 @@ func mapAuthorityToAuthorityProperty(authority string) string { | |||
|
|
|||
| func mapAuthorityToIdentifierLabel(authority string) (label string) { | |||
| switch authority { | |||
| case UP_AUTHORITY: | |||
There was a problem hiding this comment.
This should probably be a map
There was a problem hiding this comment.
There's no performance improvement by changing it to a map, and I think this is slightly more readable than defining the map and then doing the lookup.
There was a problem hiding this comment.
Disagree - you're doing a key value lookup, which is exactly what maps exist for. Adding new mappings will require 2 lines for the constants, plus an addition to this switch statement, which is inefficient
concordances/cypher_test.go
Outdated
| return expected.Concordance[i].Concept.ID < expected.Concordance[j].Concept.ID | ||
| }) | ||
| sort.Slice(expected.Concordance, func(i, j int) bool { | ||
| sort.SliceStable(expected.Concordance, func(i, j int) bool { |
There was a problem hiding this comment.
Not sure I like this duplication for expected and actual concordance sorting, but can't offer an alternative (that doesn't involve implementing sort.Interface on Concordances in 3 diff ways)
There was a problem hiding this comment.
I've extracted it so at least it's not repeated for both slices.
ghost
requested a review
from
concordances/cypher_test.go
Outdated
| assert.True(t, reflect.DeepEqual(expected, actual), fmt.Sprintf("Actual aggregated concept differs from expected: Test: %v \n Expected: %v \n Actual: %v", testName, expected, actual)) | ||
| } | ||
|
|
||
| func fullConcordanceSort(concordanceList []Concordance) { |
There was a problem hiding this comment.
This is nitpicky but fullConcordanceSort should be simpler (i.e. sortConcordances). Naming it full implies theres another alternative sort func that could be used, but there isn't one.
No description provided.