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(oneOf): add new error types #1022

Merged
merged 6 commits into from Sep 26, 2021

Conversation

fedeci
Copy link
Member

@fedeci fedeci commented Apr 21, 2021

Description

This closes #956
We allow the user to choose between different error types:

  • grouped: new default behaviour that groups the errors based on the original branches.
  • legacy: old behaviour that joins the errors from different branches together
  • leastErroredOnly: returns only the group with less errors

To-do list

  • I have added tests for what I changed.
  • This pull request is ready to merge.

@@ -9,16 +9,19 @@ const dummyItem: ContextItem = { async run() {} };

export type OneOfCustomMessageBuilder = (options: { req: Request }) => any;

export type OneOfErrorType = 'grouped' | 'leastErroredOnly' | 'legacy';
Copy link
Member Author

@fedeci fedeci Apr 21, 2021

Choose a reason for hiding this comment

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

Not sure about leastErroredOnly name, any suggestion?

Copy link
Member

Choose a reason for hiding this comment

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

No better names yet...

Copy link
Member

Choose a reason for hiding this comment

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

I like the name grouped as it describes well the behaviour.
What do you think of flat instead of legacy? It sounds exactly like the opposite of grouped.

As for leastErroredOnly... still no better ideas 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

flat sounds nice to me.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e30f6f5 on fedeci:one-of-error-messages into 96638b6 on express-validator:v7-features.

@fedeci fedeci mentioned this pull request Apr 21, 2021
9 tasks
@fedeci fedeci marked this pull request as ready for review May 24, 2021 17:48
@@ -9,16 +9,19 @@ const dummyItem: ContextItem = { async run() {} };

export type OneOfCustomMessageBuilder = (options: { req: Request }) => any;

export type OneOfErrorType = 'grouped' | 'leastErroredOnly' | 'legacy';
Copy link
Member

Choose a reason for hiding this comment

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

No better names yet...

src/middlewares/one-of.spec.ts Outdated Show resolved Hide resolved
src/middlewares/one-of.ts Outdated Show resolved Hide resolved
@fedeci fedeci requested a review from gustavohenke June 23, 2021 15:10
Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

Can we add docs too?

src/middlewares/one-of.ts Outdated Show resolved Hide resolved
@@ -9,16 +9,19 @@ const dummyItem: ContextItem = { async run() {} };

export type OneOfCustomMessageBuilder = (options: { req: Request }) => any;

export type OneOfErrorType = 'grouped' | 'leastErroredOnly' | 'legacy';
Copy link
Member

Choose a reason for hiding this comment

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

I like the name grouped as it describes well the behaviour.
What do you think of flat instead of legacy? It sounds exactly like the opposite of grouped.

As for leastErroredOnly... still no better ideas 😢

Comment on lines 14 to 21
message?: OneOfCustomMessageBuilder,
options: { message: OneOfCustomMessageBuilder; errorType?: OneOfErrorType },
): Middleware & { run: (req: Request) => Promise<void> };
export function oneOf(
chains: (ValidationChain | ValidationChain[])[],
message?: any,
options: { message?: any; errorType?: OneOfErrorType },
): Middleware & { run: (req: Request) => Promise<void> };
Copy link
Member Author

Choose a reason for hiding this comment

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

Hoping that in the future typescript will help us to avoid these awful signatures.

@fedeci
Copy link
Member Author

fedeci commented Sep 26, 2021

Merging this, we still have some time to update leastErroredOnly before the release

@fedeci fedeci merged commit 29ef5c7 into express-validator:v7-features Sep 26, 2021
@fedeci fedeci deleted the one-of-error-messages branch September 26, 2021 13:46
gustavohenke pushed a commit that referenced this pull request Dec 28, 2022
gustavohenke pushed a commit that referenced this pull request Dec 28, 2022
gustavohenke pushed a commit that referenced this pull request Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants