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

Update typings.d.ts #22

Merged
merged 2 commits into from
Aug 20, 2018
Merged

Update typings.d.ts #22

merged 2 commits into from
Aug 20, 2018

Conversation

alecmev
Copy link
Contributor

@alecmev alecmev commented May 20, 2018

A bit more relaxed now.

typings.d.ts Outdated
errors: ErrorObject[],
options: betterAjvErrors.IInputOptions
errors: ErrorObject[] | null | undefined,
options: betterAjvErrors.IInputOptions | undefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the JS signature.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer

errors?: ErrorObject[],
options?: betterAjvErrors.IInputOptions

typings.d.ts Outdated
@@ -22,8 +22,8 @@ declare namespace betterAjvErrors {
(
schema: any,
data: any,
errors: ErrorObject[],
options: betterAjvErrors.IInputOptions
errors: ErrorObject[] | null | undefined,
Copy link
Contributor Author

@alecmev alecmev May 20, 2018

Choose a reason for hiding this comment

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

  1. ajv.errors / validate.errors have this type.
  2. Your code has a fallback for falsy errors, so this is safe.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

@torifat
Copy link
Collaborator

torifat commented Aug 19, 2018

Sorry for the delay for some reason I didn't get any email notifications from this repo.

@alecmev
Copy link
Contributor Author

alecmev commented Aug 19, 2018

@torifat No worries. Replaced undefined with ?, don't know why I went the verbose way.

@@ -22,8 +22,8 @@ declare namespace betterAjvErrors {
(
schema: any,
data: any,
errors: ErrorObject[],
options: betterAjvErrors.IInputOptions
errors?: ErrorObject[] | null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the null necessary?

Copy link
Contributor Author

@alecmev alecmev Aug 19, 2018

Choose a reason for hiding this comment

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

Yep! That's what made me make this PR, IIRC. Otherwise I have to assert that it isn't null first (or append a !), which is redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, since this isn't a breaking change so I'm merging it but I will probably get rid of null from code in near future. Thanks for the PR 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't follow, why allow undefined, but not null? In the link I provided, ajv's errors property on the validation function may be null.

@torifat torifat merged commit 02fa9b0 into atlassian:master Aug 20, 2018
@torifat
Copy link
Collaborator

torifat commented Aug 20, 2018

Use v0.5.3, it has your patch.

@alecmev alecmev deleted the patch-1 branch August 20, 2018 08:28
@alecmev
Copy link
Contributor Author

alecmev commented Aug 20, 2018

Thanks 👍

orgads pushed a commit to orgads/better-ajv-errors that referenced this pull request Oct 18, 2021
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.

2 participants