-
-
Notifications
You must be signed in to change notification settings - Fork 111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(dcs): add map type support for decorator command targets #722
Conversation
8d5dae2
to
508bed4
Compare
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
508bed4
to
134c20b
Compare
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. element
sounds very generic ... Perhaps something more explicitly (map specific) would help in the model? mapElement
?
Alternatively we could co-opt property
and properties
for maps, but use special property names so we don't need a model change.
property: 'MAP_KEY'
property: 'MAP_VALUE'
properties: ['MAP_KEY', 'MAP_VALUE']
Thinking about this - we will also need to update the Vocabulary
and VocabularyManager
to check that terms can be retrieved for map key and value.
Signed-off-by: Jonathan Casey <[email protected]>
Yes, I'm good with mapElement, although I like the elegance of
I am less in favour of this suggestion . Strictly speaking, Maps do not contain properties. I think we should align the language of the command to that of the models AST, so as to provide consistency and avoid confusion.
Yes this came up in a team meeting, will be implemented as part of the ongoing Vocabulary efforts. |
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
048dc8f
to
d51e5ef
Compare
packages/concerto-core/test/data/decoratorcommands/invalid-target-element.json
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very close! I think making the mapElement an enum will make the model self-describing and improve validation.
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Agree using an Enum set makes sense now that we have DecoratorSet validation changes merged earlier today. One thing to clarify - do you mean to suggest bumping the concerto version, or the namespace version to |
Validate has been present for a while - the change yesterday exposed it as a standalone method, rather than just doing it implicitly as part of decorateModels. I meant to bump to |
@dselman given we haven't yet cut a release with the |
The problem remaining at v0.2.0 for DCS model will cause is that someone can define a DCS with mapElement and use it with concerto-core v3.13.0 and it will fail validation, even though the namespace version is correct. I think downstream consumers need to migrate (on the fly or in batch) DCS docs to support what the version of concerto-core supports. |
Signed-off-by: Jonathan Casey <[email protected]>
b25603b
to
8aab804
Compare
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
e68fb70
to
2d09e53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. A couple of minor changes requested.
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bravo!
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Signed-off-by: Jonathan Casey <[email protected]>
Description
This change extends the Decorator Manager to support Map Types. A new optional property
mapElement
is added to theCommandTarget
which allows users to specify the granularity of the intended target on a Map. The legal options of which areKEY
,VALUE
, orKEY_VALUE
(both Key and Value).Example 1:
In the above sample, the
@Foo
decorator will be added to both elements for the Map DeclarationDictionary
Example 2:
In the above sample, the
@Foo
decorator will be added to the Dictionary Map Declaration, if it contains a StringMapKeyType.Example 3:
In the above sample, the
@Foo
decorator will be added to all MapDeclarations found in the ModelFile, which contain a StringMapKeyType.Author Checklist
--signoff
option of git commit.