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

Jump to bottom

Added error codes to property validation errors #835

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

Added error codes to property validation errors #835

wants to merge 4 commits into from
+44 −18

Conversation

subhajit20
Copy link
Contributor

@subhajit20 subhajit20 commented Apr 5, 2024 •
edited
Loading

Closes #834

  • removed plain error and added IllegalModelException and TypeNotFoundException

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

subhajit20
subhajit20 commented Apr 5, 2024
View reviewed changes
Copy link
Contributor Author

@subhajit20 subhajit20 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

What type of exception is this one? will it be falling under base exception @ekarademir

concerto/packages/concerto-core/lib/introspect/property.js

Lines 181 to 184 in abd1c8e

if(!parent) {
throw new Error('Property ' + this.name + ' does not have a parent.');
}
const modelFile = parent.getModelFile();

@ekarademir
Copy link
Contributor

What type of exception is this one? will it be falling under base exception @ekarademir

concerto/packages/concerto-core/lib/introspect/property.js

Lines 181 to 184 in abd1c8e

if(!parent) {
throw new Error('Property ' + this.name + ' does not have a parent.');
}
const modelFile = parent.getModelFile();

Usually IllegalModelException

@subhajit20
Copy link
Contributor Author

subhajit20 commented Apr 5, 2024 •
edited
Loading

What type of exception is this one? will it be falling under base exception @ekarademir

concerto/packages/concerto-core/lib/introspect/property.js

Lines 181 to 184 in abd1c8e

if(!parent) {
throw new Error('Property ' + this.name + ' does not have a parent.');
}
const modelFile = parent.getModelFile();

Usually IllegalModelException

Okay, Hey have added that.
Can. you once review this and give me some feedback.

ekarademir
ekarademir reviewed Apr 8, 2024
View reviewed changes
packages/concerto-core/lib/introspect/property.js Outdated Show resolved Hide resolved
packages/concerto-core/lib/introspect/property.js
@@ -178,15 +179,15 @@ class Property extends Decorated {

const parent = this.getParent();
if(!parent) {
throw new Error('Property ' + this.name + ' does not have a parent.');
throw new IllegalModelException('Property ' + this.name + ' does not have a parent.');
Copy link
Contributor

Choose a reason for hiding this comment

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

This wouldn't close #834 since these exceptions are not adding any error codes. The whole point of this, was to not rely on just the types of exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey I have added a new param called errorType in illeagalexception and basefilexception ,
Is this the correct way to add error codes? need some feedback @ekarademir

Copy link
Contributor

Choose a reason for hiding this comment

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

They should follow the class name of the exception being thrown.

@subhajit20 subhajit20 requested a review from ekarademir April 9, 2024 06:46
ekarademir
ekarademir reviewed Apr 19, 2024
View reviewed changes
Copy link
Contributor

@ekarademir ekarademir left a comment

Choose a reason for hiding this comment

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

Few more bits and pieces please

packages/concerto-core/lib/introspect/illegalmodelexception.js Outdated Show resolved Hide resolved
packages/concerto-util/lib/basefileexception.js Outdated Show resolved Hide resolved
packages/concerto-util/types/lib/basefileexception.d.ts Outdated
@@ -14,8 +14,9 @@ declare class BaseFileException extends BaseException {
* @param {string} fullMessage - the optional full message text
* @param {string} [fileName] - the file name
* @param {string} [component] - the component which throws this error
* @param {string} errorType - The optional error code regarding the error
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is generated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I have written that. Should it be removed or should I add @param {string} [errorType='BaseFileException'] - the error code ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you generate the types then please? npx lerna run build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I execute the command it generated a bunch of other type files like 81 files has been generated.
Should I commit all of them ? @ekarademir

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I think I have resolved the problem.
Generated all the required types.

@subhajit20
Generated all the required types
14ea7ac 
Signed-off-by: Subhajit Ghosh <[email protected]>
@subhajit20 subhajit20 force-pushed the issue/Add_error_codes_to_property branch from 2cd1d97 to 14ea7ac Compare April 24, 2024 15:39
@subhajit20
Added minor changes
0c19cfb 
Signed-off-by: Subhajit Ghosh <[email protected]>
@subhajit20 subhajit20 force-pushed the issue/Add_error_codes_to_property branch from 4b25498 to 0c19cfb Compare April 24, 2024 16:03
@subhajit20 subhajit20 requested a review from ekarademir April 24, 2024 16:04
ekarademir
ekarademir requested changes May 10, 2024
View reviewed changes
Copy link
Contributor

@ekarademir ekarademir left a comment

Choose a reason for hiding this comment

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

@subhajit20 it looks like it's failing linting rules, could you please update the PR?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 8, 2024

This PR is stale because it has been open 15 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekarademir ekarademir ekarademir requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add error codes to property validation errors
2 participants
@subhajit20 @ekarademir