Skip to content
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

Range Types #96

Merged
merged 13 commits into from
Jul 5, 2023
Merged

Range Types #96

merged 13 commits into from
Jul 5, 2023

Conversation

darkdrag00nv2
Copy link
Contributor

Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I like the Range and Progression types because they are nicely extensible; we could later allow people to iterate over any kind of comparable type (strings, addresses, etc) if we lift the restriction that the type argument here be an Integer. A couple of suggestions though:

I don't think Range and Progression actually need to be different types, since a Range is just a special case of the more general Progression type where the step is a specific default value (in this case 1 or -1). I suggest that we combine these into one Range type that can be given a step if the developer wishes, but otherwise uses these defaults.

Additionally, I think we don't need the specific downTo operator; all this is really doing is making the step negative.

What if instead the range was always defined with .., and the sign of the step is automatically inferred from whether the start or end value is greater. We could also imagine a syntax like let range = x, y .. z where the start of this range is x, the end is z, and the step is y-x. Rephrased, developers could define the sequence by giving it a start and end, and if they want the step to be something other than +/- 1, they can define the second element of the range and the step is automatically inferred based on that?

cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you for writing up this proposal 👍

cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range-progression.md Outdated Show resolved Hide resolved
Co-authored-by: Daniel Sainati <sainatidaniel@gmail.com>
@darkdrag00nv2
Copy link
Contributor Author

@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: onflow/cadence#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 - onflow/cadence#2482 (comment)) or keep them separate as @turbolent proposed (see onflow/cadence#2482 (comment)).

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
Copy link
Contributor

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 darkdrag00nv2 changed the title Range & Progression InclusiveRange & ExclusiveRange Jun 6, 2023
@darkdrag00nv2 darkdrag00nv2 changed the title InclusiveRange & ExclusiveRange Range Type Jun 6, 2023
Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I like this proposal a lot more after the changes, it seems like it's going to be a nice feature.

cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
@turbolent turbolent requested a review from SupunS June 16, 2023 22:00
cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
@darkdrag00nv2
Copy link
Contributor Author

darkdrag00nv2 commented Jun 19, 2023

Updated the FLIP

  • Merged InclusiveRange and InclusiveRangeWithStep into a single constructor function InclusiveRange with step being a non-required argument. Did a similar change for ExclusiveRange.
  • Renamed endInclusive and endExclusive in the two types to be end since the name of the type is indicative enough
  • Made empty ExclusiveRange creation a runtime error.
  • Renamed member field length to count

cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
@bjartek
Copy link
Contributor

bjartek commented Jun 20, 2023

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 and others added 2 commits June 20, 2023 22:56
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Co-authored-by: Bastian Müller <bastian@turbolent.com>
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall design looks good to me!

cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
@darkdrag00nv2
Copy link
Contributor Author

@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.

@darkdrag00nv2 darkdrag00nv2 requested a review from turbolent June 20, 2023 17:51
@darkdrag00nv2 darkdrag00nv2 changed the title Range Type Range Types Jun 20, 2023
@darkdrag00nv2 darkdrag00nv2 requested a review from SupunS June 21, 2023 16:41
Copy link
Member

@SupunS SupunS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM!

Copy link
Contributor

@dsainati1 dsainati1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Thanks for the hard work

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 🚀

cadence/20230602-cadence-range.md Outdated Show resolved Hide resolved
@turbolent
Copy link
Member

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 🎉

@turbolent turbolent merged commit 2ab8fdf into onflow:main Jul 5, 2023
@darkdrag00nv2 darkdrag00nv2 deleted the range_progression branch July 6, 2023 18:11
@KshitijChaudhary666 KshitijChaudhary666 mentioned this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants