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

Support complex objects as decorator argument values #984

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Mamatha1718
Copy link

@Mamatha1718 Mamatha1718 commented Jan 31, 2025

Add support for JSON objects in decorator argument

Changes

=> Added DecoratorJSON type to support key-value pairs in decorator arguments.
= >Updated parser.pegjs to parse JSON objects within decorators.
=> Enhanced validate() function to check JSON structure, ensuring keys are strings
and values are valid types (string, number and boolean).
=> Improves readability and flexibility for complex decorator arguments.

Closes

Closes #918

@DianaLease
Copy link
Member

Can you sign off your commits with DCO? And also reference the issue this closes in the PR description.

@Mamatha1718
Copy link
Author

Can you sign off your commits with DCO? And also reference the issue this closes in the PR description.

Hi @DianaLease ,

I have successfully signed off my commits with DCO and updated the PR description to reference the issue. Will you please Let me know if any further changes are needed.
Thank You.

@sanketshevkar
Copy link
Member

@Mamatha1718 I think you are building on top your older branch you were using for the analysis PR. I can see the concerto analysis changes here as well.

Can you please fix the DCO signoff as well. Please follow the steps in Contributors guidelines. --signoff during creating a commit on your branch would help you avoid the DCO signoff failure.

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.

Can you please separate the decorator related changes from the analysis related changes?

@@ -159,6 +159,25 @@ class Decorator {
this.handleError(validationOptions.invalidDecorator, err);
}
break;
case 'DecoratorJSON': {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add happy and sad path test cases covering the new code and functionality you've added.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @sanketshevkar , I have added test cases to validate the new functionality:
=> Happy Path: valid JSON decorator argument.
=> Sad Path:
JSON key is not a string
JSON value is unsupported
Argument is not an object
I also updated the beforeEach setup to properly stub getModelManager( ). I am currently facing a small issue:
"TypeError: Cannot read properties of undefined (reading "returns" )".
It seems related to sinon.stubInstance(AssetDeclaration) Any suggestions on handling this stub properly ?
Will you please let me know if further changes need.
Thank You.

@@ -917,12 +917,29 @@ DecoratorIdentifier =
...buildRange(location())
}
}

Copy link
Member

Choose a reason for hiding this comment

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

You'll have to run the codegen command to generate the parser expressions for this new grammar.

@Mamatha1718
Copy link
Author

@Mamatha1718 I think you are building on top your older branch you were using for the analysis PR. I can see the concerto analysis changes here as well.

Can you please fix the DCO signoff as well. Please follow the steps in Contributors guidelines. --signoff during creating a commit on your branch would help you avoid the DCO signoff failure.

Hi @sanketshevkar ,Sorry for the inconvenience. I have created a new PR from a clean branch and successfully signed off the DCO. Let me know if any further changes are needed. Thanks!

@Mamatha1718
Copy link
Author

Can you please separate the decorator related changes from the analysis related changes?

Hi @sanketshevkar , I separate the decorator and analysis related changes. analysis related changes move into new branch.
Thank you!

mttrbrts and others added 10 commits February 8, 2025 10:58
Signed-off-by: Matt Roberts <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>
* chore(actions): publish v3.20.1 to npm

Signed-off-by: GitHub <[email protected]>

* fix(utils): fix browser build for concerto-util

Signed-off-by: sanketshevkar <[email protected]>

* fix(utils): fix browser build for concerto-util

Signed-off-by: sanketshevkar <[email protected]>

* fix(utils): fix browser build for concerto-util

Signed-off-by: sanketshevkar <[email protected]>

---------

Signed-off-by: GitHub <[email protected]>
Signed-off-by: sanketshevkar <[email protected]>
Co-authored-by: mttrbrts <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>
wq
Signed-off-by: Mamatha Bandi <[email protected]>

Signed-off-by: Mamatha Bandi <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>

Signed-off-by: Mamatha Bandi <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>
Signed-off-by: Mamatha Bandi <[email protected]>
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.

Support complex objects as decorator argument values
4 participants