-
Notifications
You must be signed in to change notification settings - Fork 87
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
refactor: migrate decimal validator to ts #750
Conversation
This PR seems to have both the dateValidator and decimalValidator changes, can help to separate them first before I review? |
interface IIsFloatOptions { | ||
min?: number | ||
max?: number | ||
} |
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.
don't declare interfaces in a running function
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.
shifted out
const makeDecimalFloatRangeValidator: DecimalValidatorConstructor = ( | ||
decimalField, | ||
) => (response) => { | ||
const { customMin, customMax } = decimalField.ValidationOptions || {} |
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.
is the fallback to empty object truly necessary?
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 be since customMin and customMax both have defaults, but we should also double check (eg by deleting ValidationOptions on a decimal field and checking if mongoose puts it back automatically)
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 the fallback
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.
@liangyuanruo @mantariksh actually can we keep the fallback since there is really no downside; although mongoose restores the default when I tested it manually, the failing tests make me nervous
const isFloatOptions: IIsFloatOptions = {} | ||
if (customMin || customMin === 0) { | ||
isFloatOptions['min'] = customMin | ||
} | ||
if (customMax || customMax === 0) { | ||
isFloatOptions['max'] = customMax | ||
} |
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.
would this end up achieving the same logic?
const isFloatOptions = {
min: customMin,
max: customMax
}
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.
IsFloatOptions activates its options based on the presence of each property, hence it is necessary to manually add 'min' and 'max'; added a comment within
// isFloat validates range correctly for floats up to 15 decimal places | ||
// (1.999999999999999 >= 2) is False | ||
// (1.9999999999999999 >= 2) is True | ||
return isFloat(answer, isFloatOptions) |
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.
maybe the isFloatOptions
variable can be inlined into isFloat
directly?
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.
As above, no because the properties must be conditionally added
b1d8260
to
c7befe4
Compare
1f2c852
to
56df4d6
Compare
b7d21cd
to
327744f
Compare
72dfbc0
to
3bd0e54
Compare
1254e92
to
92c76e3
Compare
92c76e3
to
381efaf
Compare
To be merged in after #749
Problem
Part of #7
Solution
Improvements
Tests