Fix/issue 124

[malakai97]

Resolves #124

Constructing a spec is difficult for this issue, but the raise guard will fail before rails finishes loading if the constant is not yet defined.

[dazza-codes]

I'm not understanding the bug that this is supposed to fix. Why doesn't the existing initializer work as expected? The bug description in #124 has no details about when the bug arises or how to replicate the bug.

[dazza-codes]

Possibly related references:

• http://guides.rubyonrails.org/initialization.html
• http://stackoverflow.com/questions/21638396/custom-date-time-format-in-rails-4
• http://guides.rubyonrails.org/i18n.html#adding-date-time-formats

[dazza-codes]

At first glance, it seems that Time::DATE_FORMATS[:dpn] is only ever used in the subclasses within app/adapters and only when they call map_date. Moreover, every single call uses the same value. If this analysis is correct, this date is not a parameter at all, it's a constant that could be simply a literal value within the map_date method. Why should it be anything more complicated than that?

[dazza-codes]

Another way to refactor this is to create a private or protected method in Adapter#date_format, e.g.

class Adapter

  def map_date(model_field, public_field)
    # call the date_format method, etc.
  end

  protected
  def date_format
    "%Y-%m-%dT%H:%M:%SZ"
  end
end

This would allow sub-classes to specialize it, if required. However, none of them do.

Another idea is to define a default, e.g.

def map_date(model_field, public_field, date_format = "%Y-%m-%dT%H:%M:%SZ")
  # etc
end

[dazza-codes]

Not clear why the config/initializers/time_formats.rb exists, really. The additional method could be in an app helper or a library module:

def time_from_string(string_time)
  Time.zone.parse(string_time.gsub(/\.[0-9]*Z\Z/, "Z"))
end

Also, when we look closer at this method, it's a puzzle that it's required at all because it is trying to parse or modify a string that the parser can handle just fine, e.g.

$ rails c
Loading development environment (Rails 4.2.7.1)
irb: warn: can't alias context from irb_context.
irb(main):001:0> time_from_string("2015-02-25T15:27:40Z")
=> Wed, 25 Feb 2015 15:27:40 UTC +00:00
irb(main):002:0> Time.zone.parse("2015-02-25T15:27:40Z")
=> Wed, 25 Feb 2015 15:27:40 UTC +00:00
irb(main):003:0> 

If that's the case, this method should disappear entirely. What's the use case that it serves that is not already solved by the Time.zone.parse method? It would be wise to trust the stdlib to parse stuff correctly.

This method seems to be used in the spec/adapters code and also in the application at:

$ ack-grep 'time_from_string' app/
app/controllers/application_controller.rb
48:          params[key] = time_from_string(timestamp)

app/adapters/adapter.rb
168:      return time_from_string(time)

[dazza-codes]

If it cannot be nil, give it a default value. Use the default that is always given when this method is called, i.e.

map_date(model_field, public_field, date_format = "%Y-%m-%dT%H:%M:%SZ")

[malakai97]

At first glance, it seems that Time::DATE_FORMATS[:dpn] is only ever used in the subclasses within app/adapters and only when they call map_date. Moreover, every single call uses the same value. If this analysis is correct, this date is not a parameter at all, it's a constant that could be simply a literal value within the map_date method. Why should it be anything more complicated than that?

The Adapter/AbstractAdapter code does not need to know anything about the rest of the application other than it's built on Rails. We could extract it out into a gem. Defining a magic number specific to our application in these classes would solve our problem, but it would also violate the separation of concerns.

Not clear why the config/initializers/time_formats.rb exists, really. The additional method could be in an app helper, i.e. this one could move to app/helpers/application_helper.rb:

The goal here seems to be to move it into an application-wide top-level namespace, but it's already available in one such location. Also, despite the name, ApplicationHelper and the rest of the helpers are only available to views by default. I have no idea how the rails team didn't think to call these "view helpers."

If it cannot be nil, give it a default value.

This constant is invoked by class methods that explicitly declare the mapping between the public interface and our model. We want the class definitions to be precise, so having no default is appropriate here.

[dazza-codes]

I propose this is better fixed in #128 - as the actual date/time format being used is ISO8601 and the code in #128 makes this explicit. Also, it relies on the existing tried and tested libraries to parse/manipulate date time strings, rather than a custom method that wraps the libraries (i.e. the responsibility for date/time parsing or manipulation is not in any methods of this app).

[dazza-codes]

I don't love it, but I'm going to move forward on this to keep moving on an RC7 release. I will try to rebase and rework #128 to resolve my concerns about the code designs.