onflow / flips

Flow Improvement Proposals
26 stars 23 forks source link

Range Types #96

Closed darkdrag00nv2 closed 1 year ago

darkdrag00nv2 commented 1 year ago

Work towards https://github.com/onflow/developer-grants/issues/179

darkdrag00nv2 commented 1 year ago

@turbolent @dsainati1 Thanks for your review comments. I agree with all of the suggestions and will modify the FLIP with the following new proposal:

We will not have operators to create a *Range. We can just have standard library functions. There will be two types & constructors: InclusiveRange and ExclusiveRange. I'll revert the lexer/parser changes I made in my draft PR: https://github.com/onflow/cadence/pull/2523. This would eliminate the need of operators & all the readability concerns with it.

The only source of confusion for me is whether we want to unify Range and Progression types as @dsainati1 proposed (originally proposed - https://github.com/onflow/cadence/issues/2482#issuecomment-1546977952) or keep them separate as @turbolent proposed (see https://github.com/onflow/cadence/issues/2482#issuecomment-1548703720).

For unified, the constructors will be InclusiveRange, InclusiveRangeWithStep, ExclusiveRange & ExclusiveRangeWithStep due to lack of function overloading.

For separate, the constructors will InclusiveRange, InclusiveProgression, ExclusiveRange & ExclusiveProgression.

dsainati1 commented 1 year ago

IMO it's better to have them be one type, if only because the names are clearer. IMO the function name InclusiveRangeWithStep is much clearer than InclusiveProgression, so I prefer this naming scheme in general.

darkdrag00nv2 commented 1 year ago

Updated the FLIP

bjartek commented 1 year ago

Can we have a helper method that has some defaults? For instance for IntRange having it as step 1 is a start. Also what about specifying length and not end?

darkdrag00nv2 commented 1 year ago

@bjartek

For instance for IntRange having it as step 1 is a start.

The step argument in the constructor is optional. It already supports step of 1 or -1.

Also what about specifying length and not end?

Sounds useful but I think the argument against count from @turbolent also applies here. This might not be useful if we drop the Integer bound. So may be we can revisit and add this at a later point if there is demand.

turbolent commented 1 year ago

Let's leave this open for another week to invite additional feedback. If there's no additional feedback the FLIP is ready to get accepted and merged 🎉