Periods exclusion and merging

[AndreasBackx]

See #46. This pull request is still being worked on. I have so far added Period.exclude_from and Period.merge.

Period.exclude_from: Excludes the Period instance from the given list of Period instances.
Period.merge: Adds the instance period to a list of Period instances and removes overlaps from the list.

Period.exclude will be added in the future. I had accidentally worked on exclude_from (wanting to implement it the other way around but my brain got confused in the process) and then decided exclude_from and exclude could both be added perhaps. Period._exclude does all of the dirty work regarding exclusion.

Periods are now also sorted first based on their start attribute and then on their end attribute. This is all up for discussion, so please leave some feedback.

[sdispater]

Thanks for taking the time to make this PR :-)

However, I have some issues with it:

• I am not a fan of the exclude_from() method. It is not intuitive what it does exactly. It could be removed and you could use the merge (by modifying it) and exclude to do what it intends to do.
• I would expect the merge() method to return a Period that is an aggregation of the instance and the given periods.
• With these changes you can just do periods.merge(*periods).exclude(*periods) instead of the exclude_from() method.

Also, regarding your tests:

• Why do you declare the assertExcluded() and assertMerged() functions inside the test method, it could easily be a method of the test class.
• Also, you could declare the expected and testing values in a list and loop through them rather than repeating the same code. 

[AndreasBackx]

Thanks for the feedback! I agree with me not being very clear, I should've provided some docs in this commit too. I do have some questions regarding your issues:

• How would you aggregate a list of periods into 1 Period instance? Period.merge does the following currently:

  Input:

  [
      Period(
          start=monday,
          end=thursday
      ),
      Period(
          start=tuesday,
          end=friday
      )
  ]

  Output:

  [
      Period(
          start=monday,
          end=friday
      )
  ]

  For excluding:

  periods = [
      Period(
          start=monday,
          end=friday
      )
  ]
  Period(
      start=tuesday,
      end=thursday
  ).exclude_from(periods)

  Would result in:

  [
      Period(
          start=monday,
          end=tuesday
      ),
      Period(
          start=thursday,
          end=friday
      )
  ]

• The tests are indeed full of boilerplate, I completely agree. In the end I might just split them up in methods too like test_merge_period_in_period(self) that would test whether "merging" a Period that is inside another period would result in the latter Period being the result. These tests would then simply have a list of dicts that would be passed onto a testing method. One a bit more extensive than assertMerged/assertExcluded. The assertion methods could also be moved.

I do think merge perhaps isn't a good name in the first place. I agree that one would assume it somehow merges 2 instances, but rather it removes the redundancies from a list of periods. Do you perhaps know a better name?

Could you give some more concrete examples of how you would want to see it being implemented?

[sdispater]

Regarding the merge() method:

>>> Period(monday, thursday).merge(Period(tuesday, friday))
# Period(monday, friday)

With this, you don't need the exclude_from()method but just the exclude() one:

(Period(monday, thursday)
 .merge(Period(tuesday, friday))
 .exclude(Period(tuesday, thursday)))

[AndreasBackx]

What would happen if you did the following?

>>> Period(monday, tuesday).merge(Period(thursday, friday))

Would it just return the original one because they don't overlap?

I'll give it a shot the coming days.

[sdispater]

Yeah. I don't know for this specific case.

Anyway, I will hold off on that.

Adding exclude() and intersect() is a good idea but since the rest can be done using basic operations I don't think it should make it to the Period class. 

[AndreasBackx]

I don't think we should hold off. The PR currently has the features I need and I would definitely want to be able to do that. The merge function can be very useful for opening hours for example, which is what I use it for primarily.

Perhaps a better name for the merge function could be union or unite. It could either be classmethod and accept a list of Periods or an instance method that would add it to the given list and unite it after. What are your opinions?

[sdispater]

I understand that it is useful for you but I don't want to bloat the API right now. I want pendulum to stay simple.

That's why I see the value of adding exclude() and intersect(), they are simple, easy to understand and can serve as a base for more use cases, but for the rest I think it's not necessary to add it to the base class since it's just basic operations on Period instances anyway.

If you want these features, you can always make a separate package that include them.

[AndreasBackx]

I get that you don't want to bloat the library and I love the library for it. But I do feel that there is a benefit to having options like these, even though I agree these can be implemented differently/better. There are simply no libraries out there that can manage lists of Periods for opening hours for example. It's unfortunate and I hoped pendulum would be open for a change like that.

Thanks for the feedback. For now I'll just try to keep my fork up to date with the changes happening here, but keep these features in. Maybe in the future I'll publish it as a library when I have some more time.

[sdispater]

Know that it's not a definitive no, it's just that do not feel like integrating it in the library for now. I want to focus on stabilizing it and adding features that will make it attractible for new users.

I really appreciate the time you took in putting this PR together, I really do. And I hope it won't discourage you from proposing more of them.

[AndreasBackx]

No worries. Maybe even after using my own fork, I'll improve it by using it. The fork is always there in case people need it.