-
Notifications
You must be signed in to change notification settings - Fork 12
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: extends the input type of zonesNormalize function #267
Conversation
should we have zones if we pass only exclusion zones? and the input zones is empty? |
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.
@targos This is not a time critical code and I think it make sense to have this here.
WDYT ?
What I care about is consistency. Please also do it in the other functions of the |
Also, this seems to be a breaking change (see failing tests) |
This method in particular can returns zones with an empty zones input. e.a const result = zonesNormalize([], {
from: 0,
to: 10,
exclusions: [{ from: 1, to: 2 }],
}); this breaking change can be avoided by: if (zones.length === 0) {
zones.push({ from, to } as Zone);
} |
43c84e9
to
289c587
Compare
if (zones.length === 0) { | ||
zones.push({ from, to }); | ||
zones.push({ from, to } as Zone); |
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 is an issue because we will let think that it is a Zone but it does not have the expected properties. It could yields to bugs and this can not be accepted.
@targos I guess we could make function overload ? But I'm not sure this would really help
// Overload for when the array is empty
function test(array: []): FromTo;
// Overload for when the array is not empty
function test(array: number[]): Zone;
function test(array: number[]): FromTo | Zone {}
Otherwise we need to create a new method that just throws if the number of zones are empty
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.
Empty array does not exist in TypeScript
export function zonesWithPoints( | ||
zones: FromTo[] = [], | ||
export function zonesWithPoints<Zone extends FromTo = FromTo>( | ||
zones: Zone[] = [], |
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.
Zone is not used in this method and we don't return extended Zone (with points). Does not make sense to me.
yes, better if we need to a similar method for and more general object better to implement a new one. |
there is a package that use zonesNormalize but those zones has the structure:
{ id: string } & FromTo
so should be nice to refactor this function to accept those kind of inputs and preserve in this particular case the
id