-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Task bre params validation & default values population #466
Conversation
* Required to fix local tslint erros.
…stead of JSON strings.
* This task was already using an array param, but was being implictly handled as an string, and tests were not failing because the validation was not in place, also because the value was always an empty array thus ignored. * Now that we are implementing task arguments type validation, this will fail unless a proper type is specified and used, such as this newly implemented "array" type.
@@ -20,7 +20,8 @@ export default function() { | |||
.addOptionalVariadicPositionalParam( | |||
"testFiles", | |||
"An optional list of files to test", | |||
[] | |||
[], | |||
types.array |
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.
Variadic params are always arrays. IIRC passing a type.string
would be the right thing here.
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.
I mean, having types.array
is good, but it looses some info, as it's an array of any
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.
Removed types.array
for now since my original intent would be backwards incompatible, and it's advantages are not clear considering the current param definitions implementation. commit ce41760
/** | ||
* Provides an interface for every valid task argument type. | ||
*/ | ||
export interface ArgumentType<T> { | ||
/** | ||
* Type's name. | ||
*/ | ||
name: string; | ||
name: ArgumentTypeName; |
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.
This prevents plugin authors form creating their own types.
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.
Now that I think of, this is a breaking change also.
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.
Reverted in 91b8b8f
packages/buidler-core/src/internal/core/params/argumentTypes.ts
Outdated
Show resolved
Hide resolved
}); | ||
} | ||
|
||
return Number(strValue); | ||
}, | ||
validate: (argName: string, value: any): void => { | ||
const isFloat = Math.floor(value) !== value; |
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.
I think this validation should be type of value !== number
and possibly check for NaN
. float
should also accept integers.
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.
Fair enough, I'll fix this
@@ -1,3 +1,4 @@ | |||
/* tslint:disable:no-string-literal */ // TODO this is for unit testing priv methods. We shouldn't test these at all? |
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.
Yep, this isn't ideal, but I guess it's ok for now. If/when we modify the argument parser (probably soon, right?) we can refactor its tests.
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, but would it be worth it? Do we have plans to add further functionality regarding this topic? If it were the case maybe we should write it down as an issue/docs.
…st' task." * After PR discussion we've ended up deciding that it's unneeded, according to the current implementation logic (ie. variadic params are typed "string" even though they all make for one array of strings). * Also it changes the API so it would be a breaking change. This reverts commit df9267d.
…ations, and make it optional. * Remove from the interface to prevent introducing a breaking change. * Make it optional as well - don't validate if type is not defined, or if a custom type doesn't have a validation implemented.
…or better readability.
dfaf2ea
to
7b920bb
Compare
* Also stop wrapping type.validate() error to enforce a BuidlerError. It should already be a BuidlerError, otherwise let it bubble and be handled by some upper layer.
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.
🚀
When running a task programatically (ie. from the BuilderRuntimeEnvironment instance), validate task arguments according the constraints specified in the taskDefinition params.
This implies, running a task like this:
would now:
Fixes #191