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

Use time_ago_in_words instead of actionview #17

Closed

Conversation

etagwerker
Copy link
Contributor

Hey @jnraine,

This PR removes the actionview dependency and replaces it with time_ago_in_words

I believe this will make it easier to support this gem with older versions of ruby.

What do you think?

Thanks!

Change time_ago_in_words implementation.
Using this smaller gem will make it easier to support `ten_years_rails` with older versions of Ruby.
Copy link
Contributor

@jnraine jnraine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this fix but I'm concern that the gem we're now using is no longer maintained. Your PR gave me an idea that might support both old and new codebases well. Let me know what you think!

@@ -20,8 +20,7 @@ Gem::Specification.new do |spec|
spec.executables = spec.files.grep(%r{^exe/}) { |f| File.basename(f) }
spec.require_paths = ["lib"]

spec.add_dependency "actionview"
spec.add_dependency "activesupport"
spec.add_dependency "time_ago_in_words"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This gem appears to no longer be maintained. What if we approached this as a progressive enhancement? If actionview is available, use it. If not, just print the timestamp.

It might look something like this:

begin
  require "action_view"
rescue LoadError
  # actionview gem not installed, that's okay.
end

module DateHelpers
  if defined?(ActionView::Helpers::DateHelper)
    include ActionView::Helpers::DateHelper

    def time_ago_in_words(time)
      "#{super(time)} ago"
    end
  else
    # When ActionView isn't available, print ISO8601 formatted string
    def time_ago_in_words(time)
      time.strftime("%Y-%m-%d")
    end
  end
end

class Foo
  include DateHelpers

  def test(time)
    time_ago_in_words(time)
  end
end

puts Foo.new.test(Time.new(2010, 1, 1))

This would allow us to remove ActionView from the Gemfile while maintaining the pretty printing for those who have ActionView.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the progressive approach. I'll tweak my PR to take that into account.

Also, add `timecop` dependency to test time sensitive specs
This will make it a little easier to add test coverage for `GemInfo` and `BundleReport`

Also, I removed unused methods and made `time_ago_in_words` a little smarter so that it doesn't depend on `ActionView` helpers
@etagwerker etagwerker force-pushed the remove-action-view-dependency branch from e90f0af to 94d5ed9 Compare July 6, 2019 16:45
Copy link
Contributor Author

@etagwerker etagwerker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jnraine I just updated this PR.

In the process of improving BundleReport I thought it was best to move classes to the lib directory. This made it easier for me to add a spec for the GemInfo#age method.

Please check it out.

Thanks!

@mctaylorpants
Copy link
Contributor

Hey @etagwerker,
Since there's a lot of refactoring here and there's likely some newer changes in your upstream fork, I think I'll close this and #14 and open a new PR to backport your upstream changes (Ruby version, refactoring, etc). I'll work on this next week!

This was referenced Nov 29, 2019
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.

3 participants