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

Teach convertAjvErrors to provide an even better UX #1548

Merged
merged 6 commits into from
Nov 30, 2020
Merged

Teach convertAjvErrors to provide an even better UX #1548

merged 6 commits into from
Nov 30, 2020

Conversation

nulltoken
Copy link
Contributor

This is a follow up to #1530

  • Some paths were still not properly dealt with. This change proposal helps them render as expected. Unit tests have been added to cover those use cases.
  • When the schema requires some properties, and they've found missing in the passed in object, the error message states the name of the offending property (eg. ``). However, when the schema forbids any additional property to be added to an object, the error message is a bit unhelpful (ie. should NOT have additional properties). This change proposal adds the name of the superfluous property in the error message (eg. `should NOT have additional properties; found 'neither'`)

@@ -15,17 +15,15 @@ export const convertAjvErrors = (errors: NonEmptyArray<ErrorObject>, severity: D
errors,
map<ErrorObject, IPrismDiagnostic>(error => {
const allowedParameters = 'allowedValues' in error.params ? `: ${error.params.allowedValues.join(', ')}` : '';
const errorPath = error.dataPath.includes('.')
? error.dataPath.split('.').slice(1)
Copy link
Contributor Author

@nulltoken nulltoken Nov 25, 2020

Choose a reason for hiding this comment

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

@XVincentX I'm not quite sure where did the initial slice(1) come from. Nor what requirement it was fulfilling.
Switching to a split('.') call seems to still not break any test while making the code a bit easier to reason with.

@kikisaeba
Copy link

Hi,
I'm interested by this change.
When could it be merged ?

@XVincentX
Copy link
Contributor

Hi,

I'm interested by this change.

When could it be merged ?

We try to look at PRs in a timely manner and try to release as long as we have meaningful changes.

TL;DR: no ETA.

@nulltoken
Copy link
Contributor Author

@XVincentX 👋 Any chance you could take a look at this?

@XVincentX
Copy link
Contributor

I'll try to get on this asap; I need to do a sanity check as well as reviewing the code.

I want to make sure we're not bloating the function with edge cases and see if there are some split opportunities.

I'll keep you posted.

@XVincentX XVincentX merged commit ee37bda into stoplightio:master Nov 30, 2020
@XVincentX XVincentX self-assigned this Nov 30, 2020
@nulltoken nulltoken deleted the ntk/diagnostic_paths branch December 2, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants