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

Add DateTime handling to attribute_to_time #717

Merged
merged 1 commit into from Nov 7, 2015
Merged

Add DateTime handling to attribute_to_time #717

merged 1 commit into from Nov 7, 2015

Conversation

rosetree
Copy link
Contributor

@rosetree rosetree commented Nov 4, 2015

This enhances attribute_to_time which previously removed all time
information from a DateTime. This happened because DateTime identifies
itself as a Date.

2.2.3 :001 > require 'date'
 => true
2.2.3 :002 > DateTime.now.is_a?(Date)
 => true

Up to now, if time.is_a?(Date) is true, attribute_to_time will
create a new Time, with just year, month and day set. To fix that
behaviour, this commit adds a new condition at the beginning of
attribute_to_time, which uses time.to_time as long as time is a
DateTime.

This enhances `attribute_to_time` which previously removed all time
information from a DateTime. This happened because DateTime identifies
itself as a Date.

    2.2.3 :001 > require 'date'
     => true
    2.2.3 :002 > DateTime.now.is_a?(Date)
     => true

Up to now, if `time.is_a?(Date)` is true, `attribute_to_time` will
create a new Time, with just year, month and day set. To fix that
behaviour, this commit adds a new condition at the beginning of
`attribute_to_time`, which uses `time.to_time` as long as `time` is a
DateTime.
@denisdefreyne
Copy link
Member

Looks good!

I realised that there’s no tests for #attribute_to_time—not sure how that happened, but presumably it’s because this function is tested implicitly. If you want to, you can create a spec for this function. If not, feel free to say no—I’ll take it up soon!

@rosetree
Copy link
Contributor Author

rosetree commented Nov 5, 2015

I’d love to add tests for #attribute_to_time, but actually I’m not really sure how to do this. That shouldn’t be a huge problem, though. I’m going to learn and do this on Saturday, if that’s not to late. Maybe I’d come with some questions back to you, @ddfreyne. Should this be tested with values of example items or with static objects? Could you point me to a similar test within Nanoc, that I could use as a reference? Thanks 😺.

@denisdefreyne denisdefreyne merged commit 6f268f4 into nanoc:master Nov 7, 2015
@denisdefreyne
Copy link
Member

I went ahead and added a spec myself—hope you don’t mind! You can find the commit at df20f2e.

Some notes (for learning):

  • mod is an instance of a class that includes Nanoc::Helpers::Blogging. It’s a little unusual, because testing a module directly isn’t that simple.

  • subject defines what needs to be tested: in this case, mod.attribute_to_time(arg), where arg is the argument which takes on different values in the different contexts.

  • context defines different context—called with Time, Date, DateTime and String. If the method under test would’ve accepted an integer, with a Unix timestamp, there’d also be a context testing an int.

  • let gives arg a value.

  • it { is_expected.to eql(something) } describes what the subject (meaning the return value of #attribute_to_time called with arg—i.e. what was defined using subject) is expected to be. This is where the actual checks are. This is a shorthand form for the following:

    it "returns a correct Time instance" do
      expect(subject).to eql(something)
    end

I quite like RSpec. The Better Specs site is very helpful for making the best use of RSpec. Nanoc 3.x used Minitest, but I’m migrating parts to RSpec, and new specs are all written using RSpec.

@rosetree rosetree deleted the feature/support-datetime-in-attribute_to_time branch November 7, 2015 14:51
@rosetree
Copy link
Contributor Author

rosetree commented Nov 7, 2015

I don’t mind at all. Thank you for providing this explanation. :)

@denisdefreyne
Copy link
Member

There’s a follow-up PR here: #724 — turns out the original code did not properly tackle timezones, and Travis CI builds failed.

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

Successfully merging this pull request may close these issues.

None yet

2 participants