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

Added some Crutches.Range functions. #74

Merged
merged 1 commit into from
Aug 20, 2015
Merged

Conversation

jdl
Copy link
Contributor

@jdl jdl commented Aug 20, 2015

Range.overlaps?/2
Takes two ranges, returns true if any elements appear in both ranges.

Range.intersection/2
Takes two ranges, returns the intersection (or nil)

Range.union/2
Takes two ranges, returns the union (or nil)

Reference: #73

Note about dealing with descending ranges:

5..3 as opposed to 3..5

For the purposes of interstions and unions there's really no
difference, but the code was bugged. The results of these
functions will be ascending ranges (or nil).

Range.overlaps?/2
  Takes two ranges, returns true if any elements appear in both ranges.

Range.intersection/2
  Takes two ranges, returns the intersection (or nil)

Range.union/2
  Takes two ranges, returns the union (or nil)

Reference: mykewould#73

--
Note about dealing with descending ranges:

5..3 as opposed to 3..5

For the purposes of interstions and unions there's really no
difference, but the code was bugged. The results of these
functions will be ascending ranges (or nil).
@jdl jdl mentioned this pull request Aug 20, 2015
@duijf
Copy link
Collaborator

duijf commented Aug 20, 2015

Looks good! Props for including specs!

Some small points that might require some more discussion:

  • Is normalize_range_order a good idea? Might be good (it is at least clear what is happening in what case), but in that case we might want to provide a Range.reverse function if that doesn't already exist elsewhere. We might even want to make normalize_range_order public (maybe under a different name, such as ascend or similar).
  • Docs are clear, but it might be beneficial to add backticks to parameter names (for documentation parsers, and also Inch CI).

@jdl and @druzn3k; what do you guys think?

@CyrusNuevoDia
Copy link
Collaborator

As far as I know, the union of 2 sets is still a valid operation when dealing with non-overlapping sets.

Range.union(1..4, 10..15)
# [1..4, 10..15]

By adding that functionality are we diving down the rabbit whole of having a ComplexRange type? What do you think @duijf, @druzn3k, @jdl?

@duijf
Copy link
Collaborator

duijf commented Aug 20, 2015

@KNRZ In set theory, that is definitely defined. It might not even be a bad idea to include a type like that if there is a use case for it. I can definitely think of scheduling as an application.

Does Elixir have custom iterators or something like Python, so that we can avoid expanding those two ranges into a list if we can avoid it?

@jdl
Copy link
Contributor Author

jdl commented Aug 20, 2015

I'm not going to pretend to know the right way to handle the union of two ranges that don't overlap. However, it's possible that this is better handled as a Set function.

@duijf
Copy link
Collaborator

duijf commented Aug 20, 2015

@jdl Not an issue :) To keep you in the loop; the union operation is from set theory. As far as I'm concerned it's up for debate whether we should return nil on non overlapping ranges or whether it would be a new complex range type. Can you see that being useful to you?

If you want a set, then currently you can probably use Enum.into for that.

@jdl
Copy link
Contributor Author

jdl commented Aug 20, 2015

A complex range type seems like overkill until someone actually needs such a thing.

@duijf
Copy link
Collaborator

duijf commented Aug 20, 2015

Thanks for your input! Anyway, this looks good to me, so I'm merging it in. The rest can go on the todo list.

duijf added a commit that referenced this pull request Aug 20, 2015
Added some Crutches.Range functions. Closes #73.
@duijf duijf merged commit af4715f into mykewould:master Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants