-
Notifications
You must be signed in to change notification settings - Fork 9
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
Remove ref links & update tests #20
Conversation
Discussion: Where new message format doesn't lead to a better user experienceWhere {{description}} and {{error}} are the sameOld: Old: Old: Where {{description}} and {{error}} have redundancyOld: Old: Where meaningful information is actually removedOld: Old: Old: |
For these cases, I think we have two ways to handle them. The first one may be better choice.
|
@coliu19 Can you please add meaningful comments for above cases, as you are most familiar with code. |
@@ -39,7 +39,7 @@ describe(ruleName, () => { | |||
expect(res).toEqual([ | |||
{ | |||
code: ruleName, | |||
message: 'Date header should be type \'string\' and should not use the built-in OpenAPI format. Instead, \'pattern\' should be used to specify a custom format (https://developer.cisco.com/docs/api-insights/#!api-guidelines-analyzer)', | |||
message: 'HTTP headers follow the syntax specified in the corresponding RFCs.; "[0].pattern" property must be truthy', |
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.
Take this for example. If the message is not good. we can change the rule's description in api-insights-rulesets.js for "oas2-request-heaer-date-correct-type". to something more meaningful. Here https://github.com/cisco-developer/api-insights-openapi-rulesets/blob/am-update-analyzer-message/api-insights-openapi-ruleset.js#L698
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.
As per discussions with @npateriya & @coliu19, selectively updating message formats to address the concerns laid out here -- #20 (comment)
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
As we're now passing Spectral's raw
{{description}}; {{error}}
to the end user, we should clean up the{{error}}
by removing the unnecessary reference link as API Insights already handled mitigation/recommendation separately.As a part of this, all tests also had to be updated to test/match against the new message format.