-
Notifications
You must be signed in to change notification settings - Fork 149
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
feat: Add Range module (#7128) #7305
base: master
Are you sure you want to change the base?
Conversation
Hi @LionelMeli Thanks for your contribution. We are very busy with some big merges, but I promise we will get around to reviewing it. |
Perhaps @stephentetley could take a look, since he has experience with Haskell? Otherwise perhaps @mlutze or @JonathanStarup |
I can take a look as I'm on holiday today. One thing I noticed first I'd change order of the |
Hi all |
Hi Lionel - here are my thoughts... There are (at least) two range libraries on Hackage - I looked at the other one first - so I'd change the comment to acknowledge the particular one [1] and probably its author Robert Massaioli. I'm not so keen on the operators - initially I thought they were versions of the vector-space operators until I looked closely to see what they were doing. Once associated types are production ready in Flix it would be nice to have the vector-space / affine-space operators [2] in the standard library as they are very general, so I think we should wait for them to pick operator symbols. I'd propose the following named functions as replacements:
I didn't see the infinity shorthand constructors in Flix that are in the Haskell library so these could be added if you want to (I'm not sure about the last two):
It would be nice if [1] https://hackage.haskell.org/package/range |
Hi Stephen, I'm not implemented the lbi, ube etc operators because it's my next step... (Sorry I didn't explain in the PR). I don't know the library https://hackage.haskell.org/package/vector-space (At a glance maybe a little bit difficult for me as I'm not fuent in Haskell and Functional Programming!). No problem, It'll be just an another next step! PS: What do you think about this kind of naming (from: https://github.com/google/guava/wiki/RangesExplained) ?
|
Thanks Lionel Vector Space is quite different to Range - but it adds complementary "maths" operators to the common ones that we already have in the standard library, it would be nice if there are operators available for it (if we want the classes from it in the standard library of course). I think your proposed function names are very good - definitely better than mine. The Flix stdlib doesn't generally have abbreviated names so avoiding then is a winner, I only chose to use abbreviations to avoid long names as I was suggesting them as alternatives to operators. |
Thanks for your answers. |
@LionelMeli Why did you close this? Are we not iterating towards some implementation? :) |
Hello Magnus, the only reason is: I did wrong git commands... I reopen this one once I push the code again! Sorry for this. |
Okay, no problem! |
@LionelMeli @stephentetley Hi guys. Where are we with this? I would like to have a In particular, without knowing the specifics, it would be nice if we can have a function:
or something like this where |
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.
Looking at the code (briefly)
- The definitions of BoundType and Bound looks sensible, but they should probably be in their own files and modules.
- The definition of Range looks reasonable, if perhaps a bit over-engineered?
- What is this IteratorFactory? (I once promised myself that Flix would not have "factories" :))
- All public functions, e.g. open, closed, etc. should have docs.
- I would want to implement Iterable directly on Range, but it is unclear if I can do so?
- Perhaps we should leave out to fancy functor and map stuff. (It can come later in a subsequent PR).
- Enumerable looks like its own abstraction. Perhaps we should start by adding that in its own PR?
Addition: It would best if iteration through the array indices does not require allocation. |
I was looking at: https://typelevel.org/cats-collections/range.html and https://doc.rust-lang.org/std/ops/struct.Range.html which seems to have a simpler data type. What's the upshot of the more complex Haskell one? That is clearer? |
Hi @magnus-madsen |
|
Reopened with "Right" commits! |
No description provided.