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

Conversation

Projects
None yet
2 participants
@rosetree
Contributor

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.

Add DateTime handling to attribute_to_time
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.
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Nov 4, 2015

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!

Member

ddfreyne commented Nov 4, 2015

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!

@ddfreyne ddfreyne added this to the 4.1.0 milestone Nov 4, 2015

@rosetree

This comment has been minimized.

Show comment
Hide comment
@rosetree

rosetree Nov 5, 2015

Contributor

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 😺.

Contributor

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 😺.

@ddfreyne ddfreyne merged commit 6f268f4 into nanoc:master Nov 7, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0002%) to 98.055%
Details
@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Nov 7, 2015

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.

Member

ddfreyne commented Nov 7, 2015

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 rosetree:feature/support-datetime-in-attribute_to_time branch Nov 7, 2015

@rosetree

This comment has been minimized.

Show comment
Hide comment
@rosetree

rosetree Nov 7, 2015

Contributor

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

Contributor

rosetree commented Nov 7, 2015

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

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne Nov 8, 2015

Member

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

Member

ddfreyne commented Nov 8, 2015

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