feat(vocabulary): vocab support for map in vocab auto generation #74
Conversation
Signed-off-by: Sanket Shevkar <[email protected]>
Signed-off-by: Sanket Shevkar <[email protected]>
| const mapDeclarationName = mapDeclaration.getName(); | ||
| const mapDeclarationVocabulary = VocabularyManager.englishMissingTermGenerator(null, null, mapDeclarationName); | ||
|
|
||
| parameters.fileWriter.writeLine(0, ` - ${mapDeclarationName}: ${mapDeclarationVocabulary}`); |
There was a problem hiding this comment.
Are the whitespaces before these lines standard? i.e., if this is adding to an existing vocab yaml file, will it be at the same indentation level as the one in the yaml file?
There was a problem hiding this comment.
I don't think we support passing an existing YAML file to this. The purpose of VocabularyVistitor is just to bootstrap a new .voc file.
| @@ -54,6 +54,10 @@ class VocabularyVisitor { | |||
| return this.visitProperty(thing, parameters); | |||
| } else if (thing.isEnumValue?.()) { | |||
| return this.visitProperty(thing, parameters); | |||
| } else if (thing.isKey?.()) { | |||
| return this.visitMapKey(thing, parameters); | |||
| } else if (thing.isValue?.()) { | |||
There was a problem hiding this comment.
I guess, it's a bit late to do anything about it now, but isValue looks a lot more generic than it should be. We could have made it like isMapValue, just as we did for declarations. Just a remark, nothing actionable.
There was a problem hiding this comment.
Maybe to future-proof this, if possible, we can check if the parent of the thing is a map, os something similar to scope this to map declarations only. The same goes for the keys.
There was a problem hiding this comment.
For now I believe keys and values are only scoped to Maps as of now.
There was a problem hiding this comment.
If you are suggesting for readability and interpretation, that's why kept the visitor method names as visitMapKey visitMapValue.
I agree with you the method name is very generic.
|
@sanketshevkar as an aside, you don't necessarily need to use recursion to visit the Map Key & Value, you could do it all within the VisitMapDeclaration and keep things a bit more compact. LGTM, nice work! |
Thanks @jonathan-casey! Agreed with your suggestion. But I think we will on have two recursive calls inside a map declaration level. Shouldn't cause a big issue I believe. |
Closes #69
Changes
Related Issues
Author Checklist
--signoffoption of git commit.mainfromfork:branchname