search

nytimes / Fech

Deprecated. Please see https://github.com/dwillis/Fech for a maintained fork.
http://nytimes.github.io/Fech/
Other
114 stars 30 forks source link

Catch nil dates #70

Closed saizai closed 9 years ago

saizai commented 9 years ago

@dwillis ?

dwillis commented 9 years ago

Not sure how this differs from the existing code, which still returns nil for a nil date value. Can you give me an example that shows why it's needed?

saizai commented 9 years ago

In Rubinius:

> Date.parse(nil)
TypeError: can't dup NilClass
    from kernel/common/immediate.rb:25:in `dup'
dwillis commented 9 years ago

Ah, ok. Right now Fech doesn't support Rubinius, but I'd be open to a PR that does that, not just for date parsing, but for the entire library.

saizai commented 9 years ago

I'm using it in Rubinius right now, importing everything from the first v3 record onwards. I'll PR as needed, but so far nothing else is Rubinius related AFAICT.

dwillis commented 9 years ago

Ok - if the other specs pass in Rubinius and this is the only fix needed, that would be great.

saizai commented 9 years ago

Done. See https://github.com/NYTimes/Fech/pull/72

saizai commented 9 years ago

@dwillis Any reason not to merge this now? It's at worst equivalent to previous functionality, and fixes it on Rubinius.