-
-
Notifications
You must be signed in to change notification settings - Fork 157
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: added new evaluate command to check the score of the asyncapi document #1413
Conversation
@AayushSaini101 do we need a new command for the linked issue ? |
src/commands/evaluate.ts
Outdated
} | ||
const finalScore = (scoreEvaluate/1)*100; | ||
|
||
this.log('The score of the asyncapi document is ', +finalScore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AayushSaini101 check if we can use the new UI library for this (see #1214)
This might be used under |
Agreed, how about adding it as a flag |
IMO, I would opt for just adding a flag to |
src/commands/evaluate.ts
Outdated
if (!document?.channels().isEmpty()) { | ||
scoreEvaluate+=0.35; | ||
} | ||
const finalScore = (scoreEvaluate/1)*100; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to divide scoreEvaluate
by 1
? I guess I'm missing something 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peter-rr In the initial stage, the total weight of the coefficient is 1 that's why we need to divide the sum by 1, to get 100 Score for a valid asyncapi document.
Evaluation and Validation are closely related, having |
/update There are some issues with sonar-cloud as well @AayushSaini101. |
@Shurtu-gal @Amzani @Souvikns I have added score flag in the existing validate command, and added a test case to verify the score and command. Ready for review. : ) Thanks |
src/commands/validate.ts
Outdated
@@ -4,6 +4,24 @@ import { validate, validationFlags, ValidateOptions, ValidationStatus } from '.. | |||
import { load } from '../models/SpecificationFile'; | |||
import { specWatcher } from '../globals'; | |||
import { watchFlag } from '../flags'; | |||
import { Parser} from '@asyncapi/parser/cjs'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if you need all this code from line 7 to line 24, why you don't just use the exported function available to you in parser.ts
import { parse } from '../parser';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I missed that Done @Amzani
src/utils/parsingFile.ts
Outdated
@@ -0,0 +1,49 @@ | |||
import { load } from 'js-yaml'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have this logic in src/parser.ts
no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted thanks
src/commands/validate.ts
Outdated
@@ -35,4 +61,26 @@ export default class Validate extends Command { | |||
process.exitCode = 1; | |||
} | |||
} | |||
|
|||
private async calculateScore(filePath:string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to extract the score logic and move it under models
or utils
for now. It will make this #1200 easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to Utils Thanks
src/utils/scoreCalculator.ts
Outdated
} | ||
const finalScore = (scoreEvaluate / 1) * 100; | ||
|
||
return `The score of the asyncapi document is ${finalScore}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about returning the score instead of a log string. As this makes it easier to use later in another scenarios if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shurtu-gal In my opinion, proper logging would be better that enhanve the developer experience as well. @Amzani What do you think on this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I am not suggesting to take out the log message, just return the score from this function. And put logging where this function is consumed.
This way this function can be reused if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @Shurtu-gal thanks : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from my side. 🚀
Let's wait for @Amzani as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR! only thing missing for me is describing how score is calculated for users.
src/commands/validate.ts
Outdated
@@ -12,17 +13,27 @@ export default class Validate extends Command { | |||
help: Flags.help({ char: 'h' }), | |||
watch: watchFlag(), | |||
...validationFlags(), | |||
score: Flags.boolean({ | |||
description: 'Compute the score of the AsyncAPI document', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users might might not understand how score is calculated, could we add more description and details about that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done @Amzani
12b0927
to
0c3d245
Compare
@Shurtu-gal @Amzani Ready for review. Hopefull for the last review cycle :) |
/update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of small suggestions :)
test/integration/validate.test.ts
Outdated
@@ -245,4 +245,23 @@ describe('validate', () => { | |||
done(); | |||
}); | |||
}); | |||
// The score of the asyncapi document is 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can omit this comment and update line 262 with the correct value instead, since that line is already explanatory itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed thanks @peter-rr
Co-authored-by: Pedro Ramos <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
/u |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Quality Gate passedIssues Measures |
/rtm |
🎉 This PR is included in version 2.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Resolves: #1131