fix: improve validation-error.ts file

[peter-rr]

Description

• This fix removes the 'if' condition evaluating 'errorHasTitle' (line 42) and also removes that variable (line 37) since the 'title' property from the corresponding error must be mandatory as specified here. Therefore the evaluation of that condition is not needed. In fact, the 'title' property is already used in the previous 'if' condition (line 40).

Related issue(s)

• Discovered while working on Add convert command #41

[github-actions]

Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.

[derberg]

@peter-rr It is true we have title in ParserError under validationErrors but we do not really make sure it is used. For example look at https://github.com/asyncapi/parser-js/blob/master/lib/parser.js#L85. Can you check your change by running asyncapi validate against document that starts with asyncapi: 1.2.0? I think we still need it and just need to fix and not remove how we validate it

[peter-rr]

@derberg I've checked my changes and I'm getting the following error when running asyncapi validate against a spec file with asyncapi: 1.2.0:

ValidationError: Version 1.2.0 is not supported.

undefined

Does the undefined above mean the title is not used as you mentioned on your comment? And, if that's the case, should we need to refactor this validation-error.tsfile?

[derberg]

yes, exactly, undefined goes through because of lack of validation.

• so we still need const errorHasTitle = !!e.title;
• and we just need to check errorHasTitle when building

  errorsInfo.push(`${e.title} ${e.location.startLine}:${e.location.startColumn}`);
  

  we need one condition where errorHasTitle and errorHasLocation is true and build

  `errorsInfo.push(`${e.title} ${e.location.startLine}:${e.location.startColumn}`);`
  

  but also in case only errorHasLocation is available, we need to construct info without title

I'm just thinking now, if we could add some test 🤔 might be that in future someone else gets into this file and tries to change things as you suggested 🤔 or maybe you could add just extensive comments to the code?

[peter-rr]

Hey @derberg
Based on your suggestions, what if we consider the following conditions then to cover all the cases you mentioned:

        if (errorHasTitle && errorHasLocation) {
          errorsInfo.push(`${e.title} ${e.location.startLine}:${e.location.startColumn}`);
        } 
        if (errorHasTitle && !errorHasLocation) {
          errorsInfo.push(`${e.title}`);
        }
        if (!errorHasTitle && errorHasLocation) {
          errorsInfo.push(`${e.location.startLine}:${e.location.startColumn}`);
        }

🤔 I think we'd be adding just one more condition to the current code and it seems self-explanatory to avoid adding extensive comments. What do you think?

[derberg]

sounds good to me, the conditions

self-explanatory

yeah, I suggest not going into the trap of self-explanatory. One small comment that explains why different conditions are needed for parser errors will not harm 😄

[peter-rr]

One small comment that explains why different conditions are needed for parser errors will not harm 😄

Yeah, you're right! Ok, then I'll add a brief comment above those conditions 👌

[peter-rr]

@derberg Ready to be reviewed ^^

[derberg]

Thanks 🚀

[derberg]

@peter-rr have a look at failing linter

[peter-rr]

@derberg No more code smells now 💪

[derberg]

@peter-rr sorry for being kinda picky, but the comment that you added describe what is already visible in the if statement. What I meant by simple comment was to add short comment that this conditions are done like this because validation errors come from another library, Parser JS that does not assure that always all the fields are there in the error, so there might be cases that even title is not provided. You know what I mean? so basically add a comment that explains why conditions are needed, and not what they are doing, makes sense?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[peter-rr]

@derberg Sure, it does make sense! No worries, actually I appreciate so much this kind of corrections 🙏 They are really helpful to me 💪📚 I already pushed the correction suggested, so tell me if anything wrong or not clear enough yet :)

[derberg]

Well done 👏🏼

[derberg]

/rtm

[asyncapi-bot]

🎉 This PR is included in version 0.13.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[derberg]

@all-contributors please add @peter-rr for code

[allcontributors]

@derberg

I've put up a pull request to add @peter-rr! 🎉