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: added new evaluate command to check the score of the asyncapi document #1413

Merged
merged 24 commits into from
Jul 31, 2024

Conversation

AayushSaini101
Copy link
Contributor

@AayushSaini101 AayushSaini101 commented May 8, 2024

Resolves: #1131

  • Started implementing new command to evaluate the asyncapi file:
  • Use four parameters to describe the score of file:
 if (document?.info().hasDescription()) {
      scoreEvalate+=0.15;
    }
    if (document?.info().hasLicense()) {
      scoreEvalate+=0.25;
    }
    if (!document?.servers().isEmpty()) {
      scoreEvalate+=0.25;
    }
    if (!document?.channels().isEmpty()) {
      scoreEvalate+=0.35;
    }
    const finalScore = (scoreEvalate/1)*100;

@Shurtu-gal
Copy link
Collaborator

@AayushSaini101 do we need a new command for the linked issue ?

@AayushSaini101
Copy link
Contributor Author

#1131

I guess it would be if we add new command for this, What do you think ? @Amzani @peter-rr @Souvikns

@Shurtu-gal
Copy link
Collaborator

#1131

I guess it would be if we add new command for this, What do you think ? @Amzani @peter-rr @Souvikns

Was seeing the incorrect issue then 👍🏻

@AayushSaini101 AayushSaini101 marked this pull request as ready for review May 10, 2024 09:11
}
const finalScore = (scoreEvaluate/1)*100;

this.log('The score of the asyncapi document is ', +finalScore);
Copy link
Collaborator

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)

@Amzani
Copy link
Collaborator

Amzani commented May 13, 2024

@AayushSaini101 do we need a new command for the linked issue ?

This might be used under validate command, however, for a better DX and maintainability having a dedicated command looks like a better approach.

@Shurtu-gal
Copy link
Collaborator

@AayushSaini101 do we need a new command for the linked issue ?

This might be used under validate command, however, for a better DX and maintainability having a dedicated command looks like a better approach.

Agreed, how about adding it as a flag --evaluate. Any way parsing will be done for both validate and evaluate. Could think some more ideas 🤔.

@peter-rr
Copy link
Member

IMO, I would opt for just adding a flag to validate command since the feature to be added should work like an extension for the validation process. Also I think --score would be semantically more clear and easier to understand than --evaluate. So the resulting command could be something like asyncapi validate --score.

WDYT? @AayushSaini101 @Amzani @Shurtu-gal

if (!document?.channels().isEmpty()) {
scoreEvaluate+=0.35;
}
const finalScore = (scoreEvaluate/1)*100;
Copy link
Member

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 🤔

Copy link
Contributor Author

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.

@Amzani
Copy link
Collaborator

Amzani commented May 14, 2024

IMO, I would opt for just adding a flag to validate command since the feature to be added should work like an extension for the validation process. Also I think --score would be semantically more clear and easier to understand than --evaluate. So the resulting command could be something like asyncapi validate --score.

Evaluation and Validation are closely related, having asyncapi validate --score looks good to me as long as we can make the new feature easily maintainable, and users can discover it.

@Shurtu-gal
Copy link
Collaborator

/update

There are some issues with sonar-cloud as well @AayushSaini101.

@AayushSaini101
Copy link
Contributor Author

@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

@@ -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';
Copy link
Collaborator

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';

Copy link
Contributor Author

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/commands/validate.ts Outdated Show resolved Hide resolved
@@ -0,0 +1,49 @@
import { load } from 'js-yaml';
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted thanks

@@ -35,4 +61,26 @@ export default class Validate extends Command {
process.exitCode = 1;
}
}

private async calculateScore(filePath:string) {
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to Utils Thanks

}
const finalScore = (scoreEvaluate / 1) * 100;

return `The score of the asyncapi document is ${finalScore}`;
Copy link
Collaborator

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.

Copy link
Contributor Author

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 ?

Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @Shurtu-gal thanks : )

Shurtu-gal
Shurtu-gal previously approved these changes May 31, 2024
Copy link
Collaborator

@Shurtu-gal Shurtu-gal left a 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.

Copy link
Collaborator

@Amzani Amzani left a 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.

@@ -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',
Copy link
Collaborator

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done @Amzani

@AayushSaini101
Copy link
Contributor Author

@Shurtu-gal @Amzani Ready for review. Hopefull for the last review cycle :)

@peter-rr
Copy link
Member

/update

Copy link
Member

@peter-rr peter-rr left a 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 :)

@@ -245,4 +245,23 @@ describe('validate', () => {
done();
});
});
// The score of the asyncapi document is 50
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed thanks @peter-rr

src/core/flags/validate.flags.ts Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Jun 12, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@peter-rr peter-rr left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Copy link
Collaborator

@Amzani Amzani left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Shurtu-gal
Copy link
Collaborator

/u

Copy link
Collaborator

@Shurtu-gal Shurtu-gal left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Jul 31, 2024

@Shurtu-gal
Copy link
Collaborator

/rtm

@asyncapi-bot asyncapi-bot merged commit 0368d64 into asyncapi:master Jul 31, 2024
11 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 2.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ability to score quality of async api specification file
5 participants