String#chars vs. String#chars

[David A. Black]

Hi --

I was trying out trunk Ruby 1.9 and edge Rails, and came across the
issue that both 1.9 and ActiveSupport define String#chars. I don't
think I'm the first to notice but I did get caught on it:

$./script/console
Loading development environment (Rails 2.1.0)
>> puts "hi"
/usr/local/lib/ruby-1.9/lib/ruby/1.9.0/irb/ruby-lex.rb:191:in
`buf_input': undefined method `to_a' for "puts \"hi\"\n":String
(NoMethodError)

The problem is that irb is using String#chars, and expects it to
return an enumerator, but ActiveSupport has overridden it so that it
returns a (proxy to a) string. ActiveSupport removes the original
String#chars, like this:

module Unicode
def self.append_features(base)
if '1.8.7 and later'.respond_to?(:chars)
base.class_eval { remove_method :chars }
end

and the defines a new version. Hence the blow-up in the console
session.

I'm not seeing any solution other than changing the name of the
method. Is there another way anyone can see?

David

--
Rails training from David A. Black and Ruby Power and Light:
Intro to Ruby on Rails January 12-15 Fort Lauderdale, FL
Advancing with Rails January 19-22 Fort Lauderdale, FL *
* Co-taught with Patrick Ewing!
See http://www.rubypal.com for details and updates!

[Michael Koziarski]

> I'm not seeing any solution other than changing the name of the
> method. Is there another way anyone can see?

Yeah we're going to have to rename the unicode chars proxy method to
avoid this. We're hashing that out in this ticket:

http://rails.lighthouseapp.com/projects/8994/tickets/951-multibyte-revisited

--
Cheers

Koz

[David A. Black]

Hi --

I missed that one in my perusings -- thanks.

[Tom Lea]

Hi Core,
The following lighthouse ticket discusses an issue with the way
Time#advance works. By deferring all processing of days, and weeks to
the Date class, we loose the ability to deal in fractional days and
weeks.

As a day an a week are both absolute length of times, we can handle
fractional ones just fine. This patch deals with that.

http://rails.lighthouseapp.com/projects/8994/tickets/970

Looking for feedback on the basic idea, or if anyone has a neater way
of doing it, but +1's or a commit would be nice.

Thanks
-Tom

[Frederick Cheung]

Do you always expect 1.5 days to be == 36.hours ?

For example, if the time is 12.00 on 26/10/2008 and you're working in
london time, do you expect 1.5. days ago to be

- midnight on 25/10/2008 (which is what the current code does)
- 01:00 on 25/10/2008 (which is what subtracting 36 hours does)

In 1.2.x 1.5.days and so on were just convenience methods that
returned numbers of seconds, whereas now they are duration objects.

Fred

> Thanks
> -Tom
>
> >

[Tom Lea]

The issue is that 1.5.days.ago == 1.days.ago, at all times in all cases.
This feels very bad.
The patch should handle this in a more sensible way, moving 1 day
back, then moving 6 hours back.

I agree that the duration objects are a much more accurate apprioach
than the 1.month == 30.days at all times stuff, but this is a bug
caused by date (quite rightly) expecting to move only integer days, or
integer months.

So no, 1.5.days.ago is not the same as 36.hours.ago in all cases, but
it is the same a (1.days + 12.hours).ago in all cases.

Any clearer?

[Geoff B]

On Sep 8, 5:46 am, Tom Lea <goo...@tomlea.co.uk> wrote:
> So no, 1.5.days.ago is not the same as 36.hours.ago in all cases, but
> it is the same a (1.days + 12.hours).ago in all cases.

So, in other words, the fractional part of a day would always be
relative to a standard 24 hour day, whereas the integer part will be
sensitive to the 23 and 25 hour days that occur once per year in zones
with DST.

One issue with this setup would be:

Time.now + 1.5.days + 1.5.days

would not always return the same result as:

Time.now + 3.days

Is this edge case acceptable? Thoughts?

[Tom Lea]

This actually seems to work just fine (which was a surprise to me, and
I'll have to look into why at a later date), adding the following
tests work just fine:

def test_fractional_days_with_bst
Time.zone_default = nil
with_env_tz 'GB' do
Time.stubs(:now).returns Time.local(2008, 10, 26, 12)
assert_equal(Time.local(2008, 10, 25, 0), 1.5.days.ago)
end
end

def test_fractional_days_additional_equivilance_with_dst
with_env_tz 'GB' do
Time.stubs(:now).returns Time.local(2008, 10, 25, 12)
assert_equal(Time.now + 3.days, Time.now + 1.5.days + 1.5.days)
assert_equal(Time.now + 3.days, Time.now + (1.5.days +
1.5.days))
assert_equal(Time.now + 3.days, (Time.now + 1.5.days) +
1.5.days)
end
end

Do you have a test case that fails?

[Michael Koziarski]

This seems fine to me. It's definitely a little counter-intuitive at
first, but any other solution would likely be a more confusing. So I
think this is a net improvement for sure.

--
Cheers,

Koz

[Geoff B]

On Sep 8, 2:26 pm, Tom Lea <goo...@tomlea.co.uk> wrote:
> Do you have a test case that fails?

Actually, you're right -- it doesn't fail with Time.local instances.
But it does fail with TimeWithZone instances, as of of this commit:
http://github.com/rails/rails/commit/676d6a651497b56f31e00db4e7795669de0da672
. There's an open ticket on Lighthouse to add this same behavior to
Time.local instances: http://rails.lighthouseapp.com/projects/8994/tickets/504-rails-2-1-dst-issues

But this isn't directly relevant to you patch, other than, when this
Time.local bug (a holdover from pre-Duration days) is fixed,
Time.local(2008, 11, 2) + 1.5.hours + 1.5.hours will (correctly) no
longer be equal to Time.local(2008, 11, 2) + 3.hours.

I think Koz is right, better to have this somewhat counter-intuitive
edge case than the current behavior of ignoring the fractional part
of :days and :weeks entirely.

Given that we're fixing :days and :weeks options, would it make sense
to fix :months and :years args as well?

[Geoff B]

On Sep 8, 8:54 pm, Geoff B <gbues...@gmail.com> wrote:
> But this isn't directly relevant to you patch, other than, when this
> Time.local bug (a holdover from pre-Duration days) is fixed,
> Time.local(2008, 11, 2) + 1.5.hours + 1.5.hours will (correctly) no
> longer be equal to Time.local(2008, 11, 2) + 3.hours.

...this example is with your patch added.

[Tom Lea]

This is where it starts to get iffy... (normal?) people almost always
expect a week to be 7 days, and a day to be 24 hours, but they accept
that a month is not a fixed duration.

We could have years behave in the same way, so 0.5 years ago is 6
months ago, so we can just have:

unless options[: years].nil?
options[:years], partial_ years = options[: years].divmod(1)
options[: months] = (options[:months] || 0) + 12 *
partial_years
end

at the top, and that would be fine.... apart from a decimal number of
months does not work.... so where do we go from here:

unless options[: months].nil?
options[: months], partial_months = options[:
months].divmod(1)
options[: days] = (options[:days] || 0) + ?????? *
partial_months
end

so what goes where the ???? is now the question?

Do we try and wind the date forward by the known years, months and
days we have, then find the number of days between that date and the
same date next month?

Or do we assume 30 days and hope no one notices ;-) ?

Basically partial months are icky, and partial years depend on them,
unless we want to just assume a year is 365 days? But it feels more
natural to go the Years => Months => Days => H/M/S route (just don't
think about weeks).

We could also consider 52 weeks in a year as an option? But this still
skips out months...

The last option, is removing support for fractional months (and
years?) by making Numeric#months a method on Integer instead of
Numeric? We could even put a depreciation error message on
Numeric#months.

Which way are other people thinking?

> >

[Michael Koziarski]

> We could also consider 52 weeks in a year as an option? But this still
> skips out months...

Plus it's wrong during leap years...

>
> The last option, is removing support for fractional months (and
> years?) by making Numeric#months a method on Integer instead of
> Numeric? We could even put a depreciation error message on
> Numeric#months.
>
>
> Which way are other people thinking?

Much as it pains me to say it, the best option seems to be removing /
deprecating the use of fractional years and months. There's just not
a nice solution in either case.

--
Cheers

Koz

[Tom Lea]

Agreed. I'll get a working day/week with nuked year/month patch
knocked up in the next few days.

Any better ideas welcome in the meantime.

- tom

On 9 Sep 2008, at 21:17, "Michael Koziarski" <mic...@koziarski.com>
wrote:

[Geoff B]

On Sep 9, 3:17 pm, "Michael Koziarski" <mich...@koziarski.com> wrote:

> Much as it pains me to say it, the best option seems to be removing /
> deprecating the use of fractional years and months. There's just not
> a nice solution in either case.

If the issue is not being able to exactly specify what a fractional
part of a month or year would be, keep in mind we do have the same
issue with days and weeks in time zones that observe DST -- days will
be 23-25 hours long, and weeks will vary in length if they cross a DST
transition.

I do agree that the existing behavior of silently ignoring fractional
parts needs to be fixed, but honoring fractional parts for just :days
and :weeks, but not :months and :years, seems odd -- it would make
more sense to me to either allow fractional parts for all of these
args, and accept the imprecision (and mention it in the docs), or to
deprecate them all, and keep #advance precise.

[Michael Koziarski]

The previous imprecise behaviour of 1.month.ago was a troll magnet, I'm
loathe to start feeding those guys again...

The reality is that people almost certainly have a finite number of
these calls in their code, I don't think there'll be too many people doing:

params[:months_ago].to_f.months.ago

So deprecating or warning with fractional durations seems ok. But at
the same time (1.5.months + 2.5.months).ago can work, right? So long as
the call to advance resolves to no remainder everything's fine?

--
Cheers,

Koz

[Tom Lea]

Just warning when you pass a float with a decimal part into advance is
not very helpful, people need to know they are doing something dumb
all the time, not just when it goes wrong.

Updated the ticket to what I would like to see. I've kept the fixes to
partial days and weeks, because they are fairly predictable, and the
edge case is worth it (IMHO).

Thanks
-Tom