Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

specs for DateTime.parse #150

Merged
merged 3 commits into from

3 participants

@markburns

I initially opened rubinius/rubinius#1863 but looks like here is a better place to update the specs.

I'm adding this spec as I believe it may highlight a bug in all ruby implementations of DateTime.parse

Namely that seconds greater than 59 are parsed as 59 whereas hours, minutes, etc. with incorrect values raise an ArgumentError.

Also according to the iso8601 spec http://en.wikipedia.org/wiki/ISO_8601, 60 seconds is a valid value as it can be used to represent a leap second.

@markburns

Raised an issue on the MRI bug tracker:
https://bugs.ruby-lang.org/issues/6885

@markburns

This appears to have been addressed by ruby/ruby@280681f

and

ruby/ruby@c069246
And the issue has been marked as closed
Will update the spec accordingly

library/datetime/parse_spec.rb
@@ -3,4 +3,158 @@
describe "DateTime.parse" do
it "needs to be reviewed for spec completeness"
+
+ it "can parse a day name into a DateTime object" do

Could you update the wording to something like:

it "parses a day name into a DateTime object"

We try to keep the wording as clear and explicit as possible and not use things like it "can" or it "should". This also applies for some of the other cases described here.

Will do. By the way the issue I wrote to highlight got fixed in ruby 2.0, but I couldn't get the specs to run on my machine at the time, not sure why. So I've left this hanging.
https://github.com/rubyspec/rubyspec/pull/150/files#L0R73
I believe it now raises an ArgumentError on ruby 2.0

Ah ok. In general, if it has been acknowledged as a bug in MRI, what we do is guard it with a ruby_bug guard. Also see section 3. Bugs here:
http://rubyspec.org/guards/

Basically it is guarded and marked with the first version where it is fixed. In this case probably 2.0 then. Also we use a reference to the issue in MRI so people can find the discussion on the issue there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/datetime/parse_spec.rb
@@ -3,4 +3,158 @@
describe "DateTime.parse" do
it "needs to be reviewed for spec completeness"
+
+ it "can parse a day name into a DateTime object" do
+ d = DateTime.parse("friday")
+ d.should == DateTime.commercial(d.cwyear, d.cweek, 5)
+ end
+
+ it "can parse a month name into a Date object" do
+ d = DateTime.parse("october")
+ d.should == DateTime.civil(DateTime.send(:today).year, 10)
+ end
+
+ it "can parse a month day into a Date object" do
+ d = DateTime.parse("5th")
+ d.should == DateTime.civil(DateTime.send(:today).year, DateTime.send(:today).month, 5)

Probably better to use Date.today instead of using send() like this. Also applies to a few other cases described here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/datetime/parse_spec.rb
((34 lines not shown))
+ else
+ d.should == DateTime.civil(DateTime.send(:today).year, 4, 10)
+ end
+ end
+
+ it "can handle MMDD as month and day" do
+ d = DateTime.parse("1108")
+ d.should == DateTime.civil(DateTime.send(:today).year, 11, 8)
+ end
+
+ it "can handle YYYYMMDD as year, month and day" do
+ d = DateTime.parse("20121108")
+ d.should == DateTime.civil(2012, 11, 8)
+ end
+
+ context "YYYY-MM-DDTHH:MM:SS format" do

We don't use the context blocks in MSpec. A describe block should be fine for cases like this too.

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

Sorry for the delay, posted a few comments on some of the things to improve to the style. Hopefully you're still able to change those!

@dbussink

I'm seeing the notification in my email, but not here, confusing :S. Anyway, probably even for cases like this a ruby_bug guard seems appropriate to me, even if it won't be backported.

Cases like these can be pretty grey and often depend on judgement. You could ask for clarification from MRI and whether they want to backport. Often this is pretty much a case by case situation.

Rubyspec is definitely not abandoned, and I would love to see it be a more integral part of Ruby, as is currently being discussed.

@dbussink dbussink merged commit 1059f29 into rubyspec:master
@brixen brixen commented on the diff
library/datetime/parse_spec.rb
((40 lines not shown))
+ d = DateTime.parse("1108")
+ d.should == DateTime.civil(Date.today.year, 11, 8)
+ end
+
+ it "can handle YYYYMMDD as year, month and day" do
+ d = DateTime.parse("20121108")
+ d.should == DateTime.civil(2012, 11, 8)
+ end
+
+ describe "YYYY-MM-DDTHH:MM:SS format" do
+ it "can handle valid values" do
+ d = DateTime.parse("2012-11-08T15:43:59")
+ d.should == DateTime.civil(2012, 11, 8, 15, 43, 59)
+ end
+
+ it "can't handle invalid month values" do
@brixen Owner
brixen added a note

The description strings need to be fixed. If a method raises an exception, the description string should be, for example:

it "raises an ArgumentError when passed an invalid month"

@dbussink please do not merge specs unless they conform to the style guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brixen brixen commented on the diff
library/datetime/parse_spec.rb
((95 lines not shown))
+ d = DateTime.parse("10100")
+ d.should == DateTime.civil(2010, 4, 10)
+ end
+
+ it "can handle YYMMDD as year month and day in 1969--2068" do
+ d = DateTime.parse("201023")
+ d.should == DateTime.civil(2020, 10, 23)
+ end
+ end
+
+ it "can handle YYYYDDD as year and day number" do
+ d = DateTime.parse("1910100")
+ d.should == DateTime.civil(1910, 4, 10)
+ end
+
+ it "can handle YYYYMMDD as year and day number" do
@brixen Owner
brixen added a note

Describe what is computed. "can handle" is meaningless.

@brixen - check out https://github.com/rubyspec/rubyspec/blob/master/library/date/parse_spec.rb I used that as a guide for writing these specs. I'm happy to go over and improve the quality of any pull request I create, but you've got to be less critical and make it easier for people to be able to contribute.
I'm particularly appreciative that @dbussink has taken the time to look at and comment on this pull request and I tried to accommodate his requests. As it is the majority of pull requests have just sat ignored on this project.
I even noticed Charles Nutter's PR sat untouched for months. So I doubt it has much to do with the quality of the contributions.
I'm sure your very busy, and maybe they are not up to your high standards, but that's where someone either needs to comment to help get them improved or explain why they are being rejected. Otherwise, I'm not sure there's so much advantage in the project being open source.

@markburns I understand that using existing specs is an easy way to add new ones, sadly existing ones aren't always up to par. In this case I should have commented on these style issues when reviewing, which I regret I missed. The style guide is available at http://rubyspec.org/style_guide/, if this isn't easy to find, we should look into ways to see if we can make it more clear for people what is expected here.

I don't think Rubyspec should be less critical in this sense, I've worked with the project for quite a while and have found that unclear wording etc. can be very confusing if people come back to implement / fix certain behavior later on. I probably even say that writing good specs is often even harder than good code, which is why I feel it is important we try to uphold good standards.

In this case I missed some stuff when reviewing, that happens and I'm glad@brixen caught them. I know that there have been pull requests open for a long time, which is also why I pitched in. I am however, not trying to lower the quality bar. If you look at current open pull requests you'll see almost all of then have received feedback. I always ask people that if they feel it is stuck / not seeing progress to please remind someone. I don't always keeps tabs on every of those requests in all projects I work on, so I'd rather have people remind me once too often than too few.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 154 additions and 0 deletions.
  1. +154 −0 library/datetime/parse_spec.rb
View
154 library/datetime/parse_spec.rb
@@ -3,4 +3,158 @@
describe "DateTime.parse" do
it "needs to be reviewed for spec completeness"
+
+ it "parses a day name into a DateTime object" do
+ d = DateTime.parse("friday")
+ d.should == DateTime.commercial(d.cwyear, d.cweek, 5)
+ end
+
+ it "parses a month name into a Date object" do
+ d = DateTime.parse("october")
+ d.should == DateTime.civil(Date.today.year, 10)
+ end
+
+ it "parses a month day into a Date object" do
+ d = DateTime.parse("5th")
+ d.should == DateTime.civil(Date.today.year, Date.today.month, 5)
+ end
+
+ # Specs using numbers
+ it "can't handle a single digit" do
+ lambda{ DateTime.parse("1") }.should raise_error(ArgumentError)
+ end
+
+ it "can handle DD as month day number" do
+ d = DateTime.parse("10")
+ d.should == DateTime.civil(Date.today.year, Date.today.month, 10)
+ end
+
+ it "can handle DDD as year day number" do
+ d = DateTime.parse("100")
+ if DateTime.gregorian_leap?(Date.today.year)
+ d.should == DateTime.civil(Date.today.year, 4, 9)
+ else
+ d.should == DateTime.civil(Date.today.year, 4, 10)
+ end
+ end
+
+ it "can handle MMDD as month and day" do
+ d = DateTime.parse("1108")
+ d.should == DateTime.civil(Date.today.year, 11, 8)
+ end
+
+ it "can handle YYYYMMDD as year, month and day" do
+ d = DateTime.parse("20121108")
+ d.should == DateTime.civil(2012, 11, 8)
+ end
+
+ describe "YYYY-MM-DDTHH:MM:SS format" do
+ it "can handle valid values" do
+ d = DateTime.parse("2012-11-08T15:43:59")
+ d.should == DateTime.civil(2012, 11, 8, 15, 43, 59)
+ end
+
+ it "can't handle invalid month values" do
@brixen Owner
brixen added a note

The description strings need to be fixed. If a method raises an exception, the description string should be, for example:

it "raises an ArgumentError when passed an invalid month"

@dbussink please do not merge specs unless they conform to the style guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ lambda{DateTime.parse("2012-13-08T15:43:59")}.should raise_error(ArgumentError)
+ end
+
+ it "can't handle invalid day values" do
+ lambda{DateTime.parse("2012-12-32T15:43:59")}.should raise_error(ArgumentError)
+ end
+
+ it "can't handle invalid hour values" do
+ lambda{DateTime.parse("2012-12-31T25:43:59")}.should raise_error(ArgumentError)
+ end
+
+ it "can't handle invalid minute values" do
+ lambda{DateTime.parse("2012-12-31T25:43:59")}.should raise_error(ArgumentError)
+ end
+
+ #this seems like unexpected behaviour why not raise an ArgumentError?
+ #also according to ISO8601 seconds can be 60 to allow for a leap second
+ it "truncates seconds down to 59" do
+ d = DateTime.parse("2012-11-08T15:43:61")
+
+ d.should == DateTime.civil(2012, 11, 8, 15, 43, 59)
+ end
+
+ end
+
+ ruby_version_is "" ... "1.9" do
+ it "can handle YYDDD as year and day number" do
+ d = DateTime.parse("10100")
+ d.should == DateTime.civil(10, 4, 10)
+ end
+
+ it "can handle YYMMDD as year month and day" do
+ d = DateTime.parse("201023")
+ d.should == DateTime.civil(20, 10, 23)
+ end
+ end
+
+ ruby_version_is "1.9" do
+ it "can handle YYDDD as year and day number in 1969--2068" do
+ d = DateTime.parse("10100")
+ d.should == DateTime.civil(2010, 4, 10)
+ end
+
+ it "can handle YYMMDD as year month and day in 1969--2068" do
+ d = DateTime.parse("201023")
+ d.should == DateTime.civil(2020, 10, 23)
+ end
+ end
+
+ it "can handle YYYYDDD as year and day number" do
+ d = DateTime.parse("1910100")
+ d.should == DateTime.civil(1910, 4, 10)
+ end
+
+ it "can handle YYYYMMDD as year and day number" do
@brixen Owner
brixen added a note

Describe what is computed. "can handle" is meaningless.

@brixen - check out https://github.com/rubyspec/rubyspec/blob/master/library/date/parse_spec.rb I used that as a guide for writing these specs. I'm happy to go over and improve the quality of any pull request I create, but you've got to be less critical and make it easier for people to be able to contribute.
I'm particularly appreciative that @dbussink has taken the time to look at and comment on this pull request and I tried to accommodate his requests. As it is the majority of pull requests have just sat ignored on this project.
I even noticed Charles Nutter's PR sat untouched for months. So I doubt it has much to do with the quality of the contributions.
I'm sure your very busy, and maybe they are not up to your high standards, but that's where someone either needs to comment to help get them improved or explain why they are being rejected. Otherwise, I'm not sure there's so much advantage in the project being open source.

@markburns I understand that using existing specs is an easy way to add new ones, sadly existing ones aren't always up to par. In this case I should have commented on these style issues when reviewing, which I regret I missed. The style guide is available at http://rubyspec.org/style_guide/, if this isn't easy to find, we should look into ways to see if we can make it more clear for people what is expected here.

I don't think Rubyspec should be less critical in this sense, I've worked with the project for quite a while and have found that unclear wording etc. can be very confusing if people come back to implement / fix certain behavior later on. I probably even say that writing good specs is often even harder than good code, which is why I feel it is important we try to uphold good standards.

In this case I missed some stuff when reviewing, that happens and I'm glad@brixen caught them. I know that there have been pull requests open for a long time, which is also why I pitched in. I am however, not trying to lower the quality bar. If you look at current open pull requests you'll see almost all of then have received feedback. I always ask people that if they feel it is stuck / not seeing progress to please remind someone. I don't always keeps tabs on every of those requests in all projects I work on, so I'd rather have people remind me once too often than too few.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ d = DateTime.parse("19101101")
+ d.should == DateTime.civil(1910, 11, 1)
+ end
+end
+
+ruby_version_is "1.8.7" do
+ describe "DateTime.parse(.)" do
+ it "parses a YYYY.MM.DD string into a Date object" do
+ d = DateTime.parse("2007.10.01")
+ d.year.should == 2007
+ d.month.should == 10
+ d.day.should == 1
+ end
+
+ it "parses a DD.MM.YYYY string into a Date object" do
+ d = DateTime.parse("10.01.2007")
+ d.year.should == 2007
+ d.month.should == 1
+ d.day.should == 10
+ end
+
+ ruby_version_is "" ... "1.9" do
+ it "parses a YY.MM.DD string into a Date object" do
+ d = DateTime.parse("10.01.07")
+ d.year.should == 10
+ d.month.should == 1
+ d.day.should == 7
+ end
+ end
+
+ ruby_version_is "1.9" do
+ it "parses a YY.MM.DD string into a Date object" do
+ d = DateTime.parse("10.01.07")
+ d.year.should == 2010
+ d.month.should == 1
+ d.day.should == 7
+ end
+ end
+
+ it "parses a YY.MM.DD string into a Date object using the year digits as 20XX" do
+ d = DateTime.parse("10.01.07", true)
+ d.year.should == 2010
+ d.month.should == 1
+ d.day.should == 7
+ end
+ end
+
end
Something went wrong with that request. Please try again.