search

rubyworks / facets

Ruby Facets
http://rubyworks.github.com/facets
Other
798 stars 99 forks source link

Support homogeneous Date, DateTime and Time comparisons. #261

Closed ioquatix closed 8 years ago

ioquatix commented 8 years ago

Allows Times, Dates and DateTimes to be compared with each other. This is useful if you've got an array of Date, DateTime and Time instances and want to sort them correctly.

ioquatix commented 8 years ago

@trans I have rebased on master.

trans commented 8 years ago

This looks good too, but three points.

  1. This belongs in the standard library's Date extensions, "facets/lib/standard/facets/date/cmp.rb" The reason is because Date and DateTime are not core classes, and have to be loaded from the standard library. Without that, as given, calling Time#<=> will raise an "uninitialized constant" error. Also, be sure to add a require for it to "facets/lib/standard/date.rb". (Eventually I will probably break that file up into separate method files.)
  2. Move the tests to "test/standard/date/" per above, and also add a case for DateTime <=> Time, just to be one the safe side.
  3. Do you think we need to keep old_compare? If so then give a more normal name like cmp or something. If not then we can either do some metaprogramming tricks to get ride of it altogether or, at least, name is _cmp and make it private.

Make sense?

ioquatix commented 8 years ago

Yes

ioquatix commented 8 years ago

Can you review the updated PR. Thanks.

ioquatix commented 8 years ago

Regarding point 3, I'm not sure I have a strong opinion. Personally, the function shouldn't be visible, but I don't think going out of our way to name it something ugly or unusable is a good idea.

ioquatix commented 8 years ago

According to this https://github.com/rubyworks/facets/issues/1

should it be called op_cmp.rb (the file)?

trans commented 8 years ago

Technically yes, it should. However, I have been leaning towards getting rid of the "op_" part. At first it seemed very logical, but over time I have found it never really matters. Even if there were a useful "cmp" method, for example, it would be a rather minor "discrepancy" (for lack of a better word) that it is in the same file with #<=>. And as I said I can't think of one case where it made an any important difference because the symbols and names tend to always be closely related in function.

I am open to opinions on the matter.

ioquatix commented 8 years ago

Okay, well I agree with what you are thinking.

trans commented 8 years ago

Damn. I thought you might argue against it ;-) Which would be perfectly fine. I am really on the fence about it which is why I haven't fully committed one way or the other.

ioquatix commented 8 years ago

Well, I don't see any strong argument either way. But I do find "op_cmp" as in require 'foo/bar/op_cmp' a bit ugly.