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.

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(decorator): added new methods to extract vocab or non-vocab decorators from model #888

Merged

Conversation

DibyamAgrawal
Copy link
Contributor

@DibyamAgrawal DibyamAgrawal commented Jul 30, 2024

Added methods to extract vocab/non-vocab decorators, fixed vocabulary parsing bugs, and exposed locale validation method.

Changes

  • Added new methods to extract vocab or non-vocab decorators from model
  • Fixed some minor bugs related to vocabulary parsing
  • Exposed method to validate locale

Flags

Screenshots or Video

Related Issues

Author Checklist

  • Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
  • Vital features and changes captured in unit and/or integration tests
  • Commits messages follow AP format
  • Extend the documentation, if necessary
  • Merging to main from fork:branchname

…rators from model and fixed some bugs

Signed-off-by: Dibyam Agrawal <[email protected]>
Signed-off-by: Dibyam Agrawal <[email protected]>
Signed-off-by: Dibyam Agrawal <[email protected]>
Copy link
Member

@sanketshevkar sanketshevkar left a comment

Choose a reason for hiding this comment

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

Have you added unit tests to cover the changes that solve the bug fixes?

@DibyamAgrawal
Copy link
Contributor Author

Have you added unit tests to cover the changes that solve the bug fixes?

Updated the exiting test to check the same by adding the scenarios in the extract-test.cto file.

@sanketshevkar sanketshevkar requested review from mttrbrts, dselman and a team August 2, 2024 06:21
@sanketshevkar sanketshevkar self-requested a review August 6, 2024 04:18
@sanketshevkar
Copy link
Member

LGTM!

*/
isVocabDecorator(decoractorName) {
const vocabPattern = /^Term_/;
return decoractorName === 'Term' || vocabPattern.test(decoractorName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if you recreate the RegEx object at each call, it would be slower than just using startsWith. Is there a reason you want to put the vocabPattern in the function body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, we can use startWith instead of regex. Earlier it was having /^Term_/i as vocabPattern to check it in case insensitive manner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Contributor

Choose a reason for hiding this comment

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

You could have also move the regex to constructor and use the compiled regex. There are trade offs using either

@@ -105,18 +128,23 @@ class DecoratorExtractor {
Object.keys(vocabObject).forEach(decl =>{
if (vocabObject[decl].term){
strVoc += ` - ${decl}: ${vocabObject[decl].term}\n`;
const otherProps = Object.keys(vocabObject[decl]).filter((str)=>str !== 'term' && str !== 'propertyVocabs');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a yaml library that we can use instead of directly parsing the string? Maybe not for this PR but that would help a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can directly create the obj in expected yaml format and use the yaml library to stringify the object or parse the string to yaml obj.

filterDecorators(decorators){
if(this.removeDecoratorsFromModel){
if (this.action === DecoratorExtractor.Action.EXTRACT_ALL){
return undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this return all the decorators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if removeDecoratorsFromModel is true then it should return undefined to remove all the decorators from the model

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a disconnect between the Action and the logic then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The action parameter specifies what to extract from the model (vocab decorators, non-vocab decorators, or both). Meanwhile, removeDecoratorsFromModel flag determines if the decorators should be deleted or not from the model itself after extraction.

Copy link
Contributor

Choose a reason for hiding this comment

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

This function then can be called filterOutDecorators. And then, I think you can just return early if this.removeDecoratorsFromModel is false with the decorators and avoid wrapping the whole function definition in an if statement.

Otherwise it looks like the code is looking like doing the opposite of what the function is aiming to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Comment on lines 309 to 311
return decorators.filter((dcs) => {
return !this.isVocabDecorator(dcs.name);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return decorators.filter((dcs) => {
return !this.isVocabDecorator(dcs.name);
});
return decorators.filter((dcs) => !this.isVocabDecorator(dcs.name);
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated similar changes

@ekarademir ekarademir merged commit ad780d0 into accordproject:main Aug 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants