Repository: arrow-py/arrow

Issue:
Wrong result when a shift involves a DST change.
In paris in 2013, we changed to DST in 2013-03-31 (at 2 AM it was 3 AM), from UTC+1 to UTC+2 (DST)

Here's an example of the issue I have with arrow : 

``` python
>>> import arrow, datetime

>>> just_before = arrow.get(datetime.datetime(2013, 03, 31, 1, 50, 45), "Europe/Paris").ceil("hour")
>>> just_before
<Arrow [2013-03-31T01:59:59.999999+01:00]>

>>> just_after = just_before.replace(microseconds=+1)
>>> just_after
<Arrow [2013-03-31T02:00:00+02:00]>
>>> # That is not right... It should be either 2 AM UTC+1 or preferably  3AM UTC +2
>>> # One could argue that depending on what you ask for, it may be the correct answer, but then the following is :

>>> (just_after - just_before).total_seconds()
1e-06
>>> # That is right but not consistent with the dates

>>> (just_after.to("utc") - just_before.to("utc").total_seconds()
-3599.999999
# That should be the same value as the previous computation. Plus, the value is negative because :

>>> just_before.to('utc'), just_after.to('utc')
(<Arrow [2013-03-31T00:59:59.999999+00:00]>,
 <Arrow [2013-03-31T00:00:00+00:00]>)
```

I think the problem is pretty clear...


Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=h1) Report
> Merging [#845](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=desc) into [master](https://codecov.io/gh/arrow-py/arrow/commit/3807a1639319f341d7b16fe26bd413ef25b857c8?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/arrow-py/arrow/pull/845/graphs/tree.svg?width=650&height=150&src=pr&token=4EkGnFsIGn)](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master      #845   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            9         9           
  Lines         1785      1790    +5     
  Branches       307       308    +1     
=========================================
+ Hits          1785      1790    +5     
```


| [Impacted Files](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [arrow/arrow.py](https://codecov.io/gh/arrow-py/arrow/pull/845/diff?src=pr&el=tree#diff-YXJyb3cvYXJyb3cucHk=) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=footer). Last update [3807a16...78f00a6](https://codecov.io/gh/arrow-py/arrow/pull/845?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

systemcatch: > First pass looks solid. Is this seriously all the places where we need to cover imaginary dates/times?

I think so, by fixing `shift` we can use it in `range` and `span` which then propagate to other methods.

> Does this address https://github.com/arrow-py/arrow/issues/686 as well? If so, we should add a regression test for it.

No and looking at it I don't think that issue can be fixed in arrow. Those comparison results are baked into datetime itself.
systemcatch: @pganssle any thoughts to add here?
systemcatch: > Should we also resolve imaginary datetimes if they are constructed via the `arrow.get` or `arrow.Arrow`?

I tend to think not as `datetime` lets you construct imaginary instances directly. Users might have a reason for needing them and with the new `imaginary` property it's easy to check it
systemcatch: Hey @jadchaar I've made the change you suggested, let me know if you're happy and I will merge this in.
jadchaar: Looks great! Ship it!

Patch:
diff --git a/arrow/arrow.py b/arrow/arrow.py
index 8c442c9..79b64a4 100644
--- a/arrow/arrow.py
+++ b/arrow/arrow.py
@@ -381,7 +381,7 @@ class Arrow(object):
             yield current
 
             values = [getattr(current, f) for f in cls._ATTRS]
-            current = cls(*values, tzinfo=tzinfo) + relativedelta(
+            current = cls(*values, tzinfo=tzinfo).shift(
                 **{frame_relative: relative_steps}
             )
 
@@ -439,17 +439,17 @@ class Arrow(object):
         floor = self.__class__(*values, tzinfo=self.tzinfo)
 
         if frame_absolute == "week":
-            floor = floor + relativedelta(days=-(self.isoweekday() - 1))
+            floor = floor.shift(days=-(self.isoweekday() - 1))
         elif frame_absolute == "quarter":
-            floor = floor + relativedelta(months=-((self.month - 1) % 3))
+            floor = floor.shift(months=-((self.month - 1) % 3))
 
-        ceil = floor + relativedelta(**{frame_relative: count * relative_steps})
+        ceil = floor.shift(**{frame_relative: count * relative_steps})
 
         if bounds[0] == "(":
-            floor += relativedelta(microseconds=1)
+            floor = floor.shift(microseconds=+1)
 
         if bounds[1] == ")":
-            ceil += relativedelta(microseconds=-1)
+            ceil = ceil.shift(microseconds=-1)
 
         return floor, ceil
 
@@ -718,11 +718,17 @@ class Arrow(object):
 
     @property
     def ambiguous(self):
-        """ Returns a boolean indicating whether the :class:`Arrow <arrow.arrow.Arrow>` object is ambiguous"""
+        """ Returns a boolean indicating whether the :class:`Arrow <arrow.arrow.Arrow>` object is ambiguous."""
 
         return dateutil_tz.datetime_ambiguous(self._datetime)
 
-    # mutation and duplication
+    @property
+    def imaginary(self):
+        """Indicates whether the :class: `Arrow <arrow.arrow.Arrow>` object exists in the current timezone."""
+
+        return not dateutil_tz.datetime_exists(self._datetime)
+
+    # mutation and duplication.
 
     def clone(self):
         """Returns a new :class:`Arrow <arrow.arrow.Arrow>` object, cloned from the current one.
@@ -836,6 +842,9 @@ class Arrow(object):
 
         current = self._datetime + relativedelta(**relative_kwargs)
 
+        if not dateutil_tz.datetime_exists(current):
+            current = dateutil_tz.resolve_imaginary(current)
+
         return self.fromdatetime(current)
 
     def to(self, tz):


Test patch:
diff --git a/tests/test_arrow.py b/tests/test_arrow.py
index 3268a9a..6fda5ca 100644
--- a/tests/test_arrow.py
+++ b/tests/test_arrow.py
@@ -7,6 +7,7 @@ import sys
 import time
 from datetime import date, datetime, timedelta
 
+import dateutil
 import pytest
 import pytz
 import simplejson as json
@@ -310,6 +311,14 @@ class TestArrowAttribute:
 
         assert ambiguous_dt.ambiguous
 
+    def test_getattr_imaginary(self):
+
+        assert not self.now.imaginary
+
+        imaginary_dt = arrow.Arrow(2013, 3, 31, 2, 30, tzinfo="Europe/Paris")
+
+        assert imaginary_dt.imaginary
+
 
 @pytest.mark.usefixtures("time_utcnow")
 class TestArrowComparison:
@@ -838,6 +847,82 @@ class TestArrowShift:
             2013, 5, 5, 12, 30, 45, 1
         )
 
+    def test_shift_positive_imaginary(self):
+
+        # Avoid shifting into imaginary datetimes, take into account DST and other timezone changes.
+
+        new_york = arrow.Arrow(2017, 3, 12, 1, 30, tzinfo="America/New_York")
+        assert new_york.shift(hours=+1) == arrow.Arrow(
+            2017, 3, 12, 3, 30, tzinfo="America/New_York"
+        )
+
+        # pendulum example
+        paris = arrow.Arrow(2013, 3, 31, 1, 50, tzinfo="Europe/Paris")
+        assert paris.shift(minutes=+20) == arrow.Arrow(
+            2013, 3, 31, 3, 10, tzinfo="Europe/Paris"
+        )
+
+        canberra = arrow.Arrow(2018, 10, 7, 1, 30, tzinfo="Australia/Canberra")
+        assert canberra.shift(hours=+1) == arrow.Arrow(
+            2018, 10, 7, 3, 30, tzinfo="Australia/Canberra"
+        )
+
+        kiev = arrow.Arrow(2018, 3, 25, 2, 30, tzinfo="Europe/Kiev")
+        assert kiev.shift(hours=+1) == arrow.Arrow(
+            2018, 3, 25, 4, 30, tzinfo="Europe/Kiev"
+        )
+
+        # Edge case, the entire day of 2011-12-30 is imaginary in this zone!
+        apia = arrow.Arrow(2011, 12, 29, 23, tzinfo="Pacific/Apia")
+        assert apia.shift(hours=+2) == arrow.Arrow(
+            2011, 12, 31, 1, tzinfo="Pacific/Apia"
+        )
+
+    def test_shift_negative_imaginary(self):
+
+        new_york = arrow.Arrow(2011, 3, 13, 3, 30, tzinfo="America/New_York")
+        assert new_york.shift(hours=-1) == arrow.Arrow(
+            2011, 3, 13, 3, 30, tzinfo="America/New_York"
+        )
+        assert new_york.shift(hours=-2) == arrow.Arrow(
+            2011, 3, 13, 1, 30, tzinfo="America/New_York"
+        )
+
+        london = arrow.Arrow(2019, 3, 31, 2, tzinfo="Europe/London")
+        assert london.shift(hours=-1) == arrow.Arrow(
+            2019, 3, 31, 2, tzinfo="Europe/London"
+        )
+        assert london.shift(hours=-2) == arrow.Arrow(
+            2019, 3, 31, 0, tzinfo="Europe/London"
+        )
+
+        # edge case, crossing the international dateline
+        apia = arrow.Arrow(2011, 12, 31, 1, tzinfo="Pacific/Apia")
+        assert apia.shift(hours=-2) == arrow.Arrow(
+            2011, 12, 31, 23, tzinfo="Pacific/Apia"
+        )
+
+    @pytest.mark.skipif(
+        dateutil.__version__ < "2.7.1", reason="old tz database (2018d needed)"
+    )
+    def test_shift_kiritimati(self):
+        # corrected 2018d tz database release, will fail in earlier versions
+
+        kiritimati = arrow.Arrow(1994, 12, 30, 12, 30, tzinfo="Pacific/Kiritimati")
+        assert kiritimati.shift(days=+1) == arrow.Arrow(
+            1995, 1, 1, 12, 30, tzinfo="Pacific/Kiritimati"
+        )
+
+    @pytest.mark.skipif(
+        sys.version_info < (3, 6), reason="unsupported before python 3.6"
+    )
+    def shift_imaginary_seconds(self):
+        # offset has a seconds component
+        monrovia = arrow.Arrow(1972, 1, 6, 23, tzinfo="Africa/Monrovia")
+        assert monrovia.shift(hours=+1, minutes=+30) == arrow.Arrow(
+            1972, 1, 7, 1, 14, 30, tzinfo="Africa/Monrovia"
+        )
+
 
 class TestArrowRange:
     def test_year(self):
@@ -1027,6 +1112,18 @@ class TestArrowRange:
         for r in result:
             assert r.tzinfo == tz.gettz("US/Central")
 
+    def test_imaginary(self):
+        # issue #72, avoid duplication in utc column
+
+        before = arrow.Arrow(2018, 3, 10, 23, tzinfo="US/Pacific")
+        after = arrow.Arrow(2018, 3, 11, 4, tzinfo="US/Pacific")
+
+        pacific_range = [t for t in arrow.Arrow.range("hour", before, after)]
+        utc_range = [t.to("utc") for t in arrow.Arrow.range("hour", before, after)]
+
+        assert len(pacific_range) == len(set(pacific_range))
+        assert len(utc_range) == len(set(utc_range))
+
     def test_unsupported(self):
 
         with pytest.raises(AttributeError):