-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix(common): fix custom multi file validator #12096
fix(common): fix custom multi file validator #12096
Conversation
hello I want to write custom validator for multi files, I get exception type checking in multi files everything is ok for uploading single file |
Pull Request Test Coverage Report for Build d27548e2-674d-4694-90b7-bd77ea4d9ad3
💛 - Coveralls |
@@ -16,7 +16,7 @@ export abstract class FileValidator< | |||
* Indicates if this file should be considered valid, according to the options passed in the constructor. | |||
* @param file the file from the request object | |||
*/ | |||
abstract isValid(file?: TFile): boolean | Promise<boolean>; | |||
abstract isValid(file?: TFile | TFile[] | Record<string, TFile[]>): boolean | Promise<boolean>; |
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.
shouldn't we move this to the L11 instead as
TFile extends IFile | TFile[] | Record<string, TFile[]
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.
@micalevisk I think your idea is good thank you for your answer
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.
actually, that won't work the same because we need to use IFile[]
, not TFile[]
, as in the current version. I just tested. Also, extends FileValidator<MyFile>
looks better than extends FileValidator<MyFile[]>
could you please revert the commit a7c59
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.
Do you think first commit is ok?
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.
Yes
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.
@micalevisk i reverted to the a7c59 , is it ok right now?
This reverts commit a7c5963.
LGTM |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information