Full support for Mongoid support + by_quarter + by_calendar_month + fixes for using Date class #14

Closed
wants to merge 17 commits into
from

Conversation

Projects
None yet
2 participants
Collaborator

johnnyshields commented Jan 5, 2013

  1. Added support for Mongoid (adapter for MongoDB). It's basically a monkey-patch file (see: lib/mongoid/by_star.rb) with a small change at the end of lib/by_star.rb to detect inclusion of ActiveRecord and/or Mongoid. I refactored the specs so that the exact same specs can be run by both Mongoid and ActiveRecord seamlessly (core specs now moved to /spec/by_star/generic).
  2. Added 'by_quarter' to select by 3-month quarter of year
  3. Added 'by_calendar_month', similar to 'by_month' but additionally includes any results which fall in the same week as the first and last of the month. Useful for working with UI calendars which show rows of weeks
  4. Some fixes for using Date instead of Time as the input to by_star
  5. Added 'between_times' as alias for 'between', since 'between' method name may be used by other gems.
  6. Tests for all changes included. Note the only tests which are not passing are those which were not_passing before this commit.
  7. Bumped minor version number.
  8. Updated Readme.
  9. Added support for field-name aliases, which is a Mongo-specific feature (e.g. 'c_at' instead of 'created_at', since Mongo stores column name in each row by design)

johnnyshields added some commits Jan 5, 2013

Added support for Mongoid (adapter for MongoDB). It's basically a mon…
…key-patch file (see: lib/mongoid/by_star.rb) with a small change at the end of lib/by_star.rb to detect inclusion of ActiveRecord and/or Mongoid.

I refactored the specs so that the exact same specs can be run by both Mongoid and ActiveRecord seamlessly (core specs now moved to /spec/by_star/generic). I am able to pass 49/60 ActiveRecord specs (i.e. prior to this commit; I have not introduced any regressions) and 50/60 Mongoid test--basically the same ones fail, the only exception is "by year/should be able to order the result set" which I believe is genuinely broken on Mongoid due to API differences.

Updated readme as well.
Moved failing Mongoid case to ActiveRecord only. After investigation …
…seems the case is failing due to how the test code is written (AR-specific), not due to Mongoid features.
Collaborator

johnnyshields commented Jan 8, 2013

@radar, please take a look at this? Seems core repo (i.e. before this commit) is failing ~15% of it's tests. Can you help fix the tests in the main repo (radar/by_star), then I'll merge to my fork (johnnyshields/by_star), and finally we can merge my fork back to the head?

Owner

radar commented Jan 8, 2013

I'm sorry, I'm a little busy right now. I will look at this when I can.

Collaborator

johnnyshields commented Jan 8, 2013

Sure, sounds good.

johnnyshields added some commits Jan 27, 2013

Added #between_times as an alias of #between, since between is likely…
… to be used by other gems (it was in my case).
Added new method "by_calendar_month". This is similar to by_month, bu…
…t it also includes any results which fall in the same week as the first and last of the month. Useful for displaying monthly calendars. (This is based on a real-world use case I encountered in my app).
Bug fix: when by_month and by_week accept a Date, must convert date.t…
…o_time first.

The reason is this is that Date.end_of_week and Date.end_of_month under some conditions yield the *beginning* of the last day, rather than the end, so the query misses the last day of the period.

Probably this fix is required everywhere Date type is used.
Added new by_quarter method for 3-month quarter period. Following a s…
…imilar pattern as other methods:

x.by_quarter() gets all results from current quarter

x.by_quarter(n) gets all results from the nth quarter of current year (e.g. n=2 means Apr thru Jun). Optional :year parameter can be given.

x.by_quarter(date) gets all results from the quarter of date given
Collaborator

johnnyshields commented Jan 27, 2013

@radar know you're busy saving metropolis. Just fyi my branch is growing...

@@ -20,6 +20,7 @@ Gem::Specification.new do |s|
s.add_development_dependency "mysql2"
s.add_development_dependency "rspec-rails", "~> 2.8"
s.add_development_dependency "timecop", "~> 0.3"
+ s.add_development_dependency "mongoid", "~> 3.0" if Gem::Version.create(RUBY_VERSION.dup) >= Gem::Version.create('1.9.3')
@radar

radar Feb 2, 2013

Owner

I don't understand why the if statement is necessary on the end of this line. Could you please explain?

@johnnyshields

johnnyshields Feb 2, 2013

Collaborator

If you attempt to require Mongoid on Ruby 1.9.2 or lower I believe it throws an error. This in turn causes Travis to puke at gem load time, even though the Mongoid test cases only run if Ruby >= 1.9.3

@johnnyshields

johnnyshields Feb 16, 2013

Collaborator

@radar any comment?

README.markdown
+
+If you are using ActiveRecord, you're done!
+
+Mongoid users, please include the Mongoid::ByStar module for each model you wish to use the functionality. (This seems to be the convention among Mongoid plugins)
@radar

radar Feb 2, 2013

Owner

I would say "This is the convention among Mongoid plugins"

Collaborator

johnnyshields commented Feb 2, 2013

@radar thank you for reviewing. See comments above.

Collaborator

johnnyshields commented Apr 17, 2013

@radar been a few months now. Any way to move this forward?

Owner

radar commented Apr 17, 2013

The code looks fine, aside from a couple of failing specs that I'm sure were there before anyway. I don't have/make any time to look at them because timezone math is confusing and makes my brain hurt.

Let's merge this.

@radar radar closed this in 71d385d Apr 17, 2013

Collaborator

johnnyshields commented Apr 18, 2013

@radar thanks very much. Please cut a gem release when you are able to do so. I'll look into fixing the broken specs at some point.

Owner

radar commented Apr 22, 2013

@johnnyshields Any reason you can't just specify a git dependency?

gem 'by_star', :github => "radar/by_star", :ref => "your_favourite_ref_goes_here"

I've pushed out v2.1.0.beta2 if you can't do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment