Skip to content
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.

Sign up for GitHub

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(vocabulary): vocab support for map in vocab auto generation #74

Merged
merged 2 commits into from
Nov 22, 2023
Merged

feat(vocabulary): vocab support for map in vocab auto generation #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
63f8128
feat(vocabulary): vocab support for map in vocab auto generation
Nov 17, 2023
6ac890d
fix(package-json): change npm registry to failing tests
Nov 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion lib/codegen/fromcto/vocabulary/vocabularyvisitor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class VocabularyVisitor {
} else if (thing.isClassDeclaration?.()) {
return this.visitDeclaration(thing, parameters);
} else if (thing.isMapDeclaration?.()) {
return;
return this.visitMapDeclaration(thing, parameters);
} else if (thing.isScalarDeclaration?.()) {
return this.visitScalarDeclaration(thing, parameters);
} else if (thing.isField?.()) {
Expand All @@ -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?.()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now I believe keys and values are only scoped to Maps as of now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return this.visitMapValue(thing, parameters);
}
else {
throw new Error('Unrecognised type: ' + typeof thing + ', value: ' + util.inspect(thing, {
Expand Down Expand Up @@ -141,6 +145,30 @@ class VocabularyVisitor {
return null;
}

/**
* Visitor design pattern
* @param {MapDeclaration} mapDeclaration - the object being visited
* @param {Object} parameters - the parameter
* @return {Object} the result of visiting or null
* @private
*/
visitMapDeclaration(mapDeclaration, parameters) {
const mapDeclarationName = mapDeclaration.getName();
const mapDeclarationVocabulary = VocabularyManager.englishMissingTermGenerator(null, null, mapDeclarationName);

parameters.fileWriter.writeLine(0, ` - ${mapDeclarationName}: ${mapDeclarationVocabulary}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


parameters.fileWriter.writeLine(0, ' properties:');

const mapKey = mapDeclaration.getKey();
const mapValue = mapDeclaration.getValue();

mapKey.accept(this, parameters);
mapValue.accept(this, parameters);

return null;
}

/**
* Visitor design pattern
* @param {Property} property - the object being visited
Expand All @@ -157,6 +185,38 @@ class VocabularyVisitor {

return null;
}

/**
* Visitor design pattern
* @param {MapKeyType} mapKey - the object being visited
* @param {Object} parameters - the parameter
* @return {Object} the result of visiting or null
* @private
*/
visitMapKey(mapKey, parameters) {
const mapDeclarationName = mapKey.getParent().getName();
const mapKeyVocabulary = VocabularyManager.englishMissingTermGenerator(null, null, mapDeclarationName, 'KEY');

parameters.fileWriter.writeLine(0, ` - KEY: ${mapKeyVocabulary}`);

return null;
}

/**
* Visitor design pattern
* @param {MapValueType} mapValue - the object being visited
* @param {Object} parameters - the parameter
* @return {Object} the result of visiting or null
* @private
*/
visitMapValue(mapValue, parameters) {
const mapDeclarationName = mapValue.getParent().getName();
const mapValueVocabulary = VocabularyManager.englishMissingTermGenerator(null, null, mapDeclarationName, 'VALUE');

parameters.fileWriter.writeLine(0, ` - VALUE: ${mapValueVocabulary}`);

return null;
}
}

module.exports = VocabularyVisitor;
Loading
Loading