Skip to content
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

Use numpy datetime64 instead of datetime.date #77 #84

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,6 @@ requires = [
"wheel"
]
build-backend = "setuptools.build_meta"

[tool.mypy]
plugins = ["numpy.typing.mypy_plugin"]
1 change: 1 addition & 0 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ python_requires = >=3.8
install_requires =
python-dateutil
lark
numpy
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we merge #88 first, we'll have to move this to the pyproject.toml dependencies.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to limit this to NumPy v2, which was released in May? That was the first major release since 2006. If we do limit it to v2, we should also drop support for Python < 3.10. It looks like NumPy v2 requires Python >= 3.10.

I'm pretty sure that there are no direct Python dependencies, just some optional dependencies for speeding up operations. pip should provide pre-built wheels.

NumPy looks like it will add 30-80MB to the payload. For most applications that's not an issue. But maybe we should benchmark the current version of undate and the size with numpy? The environments that I'm typically space-constrained in are Lambdas. I think there is still a 50MB (zipped) and 250MB (unzipped) limit for Lambda functions, so this could take up a significant chunk of that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm still leaning towards using NumPy given the need to support dates outside of 9999. The other way that we could go is with struct_time, which is what python-edtf uses. But it's a little clunky and you'd still have to have a shim class (like you add here) or a conversion function.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to make NumPy optional seems like a nightmare

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking it would be nice to move to Python 3.10+ anyway, it gets us improved python type hinting and 3.9 is getting old enough at this point that it seems reasonable to drop.


[options.package_data]
* =
Expand Down
96 changes: 96 additions & 0 deletions src/undate/date.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
from enum import IntEnum

# Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None
from typing import Optional, Dict, Union
Comment on lines +1 to +4
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused imports.

The imports for Dict and Union from the typing module are not used in the file. Removing these will clean up the code and reduce confusion about dependencies.

Apply this diff to remove the unused imports:

-from typing import Optional, Dict, Union
+from typing import Optional
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
from enum import IntEnum
# Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None
from typing import Optional, Dict, Union
from enum import IntEnum
# Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None
from typing import Optional
Tools
Ruff

4-4: typing.Dict imported but unused

Remove unused import

(F401)


4-4: typing.Union imported but unused

Remove unused import

(F401)



import numpy as np

#: timedelta for single day
ONE_DAY = np.timedelta64(1, "D") # ~ equivalent to datetime.timedelta(days=1)
#: timedelta for a single year (non-leap year)
ONE_YEAR = np.timedelta64(365, "D") # ~ relativedelta(years=1)
#: timedelta for a month, assuming maximum month length (31 days)
ONE_MONTH_MAX = np.timedelta64(31, "D")


class Date(np.ndarray):
"""This class is a shim to make :class:`numpy.datetime64` act
more like the built-in python :class:`datetime.date`."""

# extend np.datetime64 datatype
# adapted from https://stackoverflow.com/a/27129510/9706217

def __new__(cls, year: int, month: Optional[int] = None, day: Optional[int] = None):
if isinstance(year, np.datetime64):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's my non-existing knowledge about typing in python, but shouldn't year be of type int per line 24? Or is datetime64 a subclass of int?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess my question is if the type of year should be a different one :)

_data = year
else:
datestr = str(year)
if month is not None:
datestr = f"{year}-{month:02d}"
if day is not None:
datestr = f"{datestr}-{day:02d}"
_data = np.datetime64(datestr)

data = np.asarray(_data, dtype="datetime64")

# expected dtype depends on date unit / how much of date is known
expected_unit = "Y"
if day is not None and month is not None:
expected_unit = "D"
elif month:
expected_unit = "M"
expected_dtype = f"datetime64[{expected_unit}]"

if data.dtype != expected_dtype:
raise Exception(
f"Unable to parse dates adequately as {expected_dtype}: {data}"
)
Comment on lines +46 to +48
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error_handling_issue
The exception raised in the __new__ method does not specify the type of error. It would be better to raise a more specific exception, such as ValueError, to indicate that the input date could not be parsed correctly.

obj = data.view(cls)
return obj
Comment on lines +17 to +50
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Improve error handling in the Date class constructor.

The constructor of the Date class is well-implemented but uses a generic Exception for error handling. It would be more informative to use a specific exception type, such as ValueError, to provide clearer feedback to the user about what went wrong.

Consider refining the exception handling with a more specific exception type:

-            raise Exception(
+            raise ValueError(
                f"Unable to parse dates adequately as {expected_dtype}: {data}"
            )
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class Date(np.ndarray):
"""This class is a shim to make :class:`numpy.datetime64` act
more like the built-in python :class:`datetime.date`."""
# extend np.datetime64 datatype
# adapted from https://stackoverflow.com/a/27129510/9706217
def __new__(cls, year: int, month: Optional[int] = None, day: Optional[int] = None):
if isinstance(year, np.datetime64):
_data = year
else:
datestr = str(year)
if month is not None:
datestr = f"{year}-{month:02d}"
if day is not None:
datestr = f"{datestr}-{day:02d}"
_data = np.datetime64(datestr)
data = np.asarray(_data, dtype="datetime64")
# expected dtype depends on date unit / how much of date is known
expected_unit = "Y"
if day is not None and month is not None:
expected_unit = "D"
elif month:
expected_unit = "M"
expected_dtype = f"datetime64[{expected_unit}]"
if data.dtype != expected_dtype:
raise Exception(
f"Unable to parse dates adequately as {expected_dtype}: {data}"
)
obj = data.view(cls)
return obj
class Date(np.ndarray):
"""This class is a shim to make :class:`numpy.datetime64` act
more like the built-in python :class:`datetime.date`."""
# extend np.datetime64 datatype
# adapted from https://stackoverflow.com/a/27129510/9706217
def __new__(cls, year: int, month: Optional[int] = None, day: Optional[int] = None):
if isinstance(year, np.datetime64):
_data = year
else:
datestr = str(year)
if month is not None:
datestr = f"{year}-{month:02d}"
if day is not None:
datestr = f"{datestr}-{day:02d}"
_data = np.datetime64(datestr)
data = np.asarray(_data, dtype="datetime64")
# expected dtype depends on date unit / how much of date is known
expected_unit = "Y"
if day is not None and month is not None:
expected_unit = "D"
elif month:
expected_unit = "M"
expected_dtype = f"datetime64[{expected_unit}]"
if data.dtype != expected_dtype:
raise ValueError(
f"Unable to parse dates adequately as {expected_dtype}: {data}"
)
obj = data.view(cls)
return obj


def Export(self):
return self

def __array_finalize__(self, obj):
if obj is None:
return

# custom properties to access year, month, day

@property
def year(self):
return int(str(self.astype("datetime64[Y]")))

@property
def month(self):
# if date unit is year, don't return a month (only M/D)
if not self.dtype == "datetime64[Y]":
return int(str(self.astype("datetime64[M]")).split("-")[-1])

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error_handling_issue
The month property does not have a return statement if the condition is not met. This could lead to returning None implicitly, which may not be the intended behavior. Consider raising an exception or returning a default value instead.

@property
def day(self):
# only return a day if date unit is in days
if self.dtype == "datetime64[D]":
return int(str(self.astype("datetime64[D]")).split("-")[-1])
Comment on lines +61 to +75
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refine logical expressions in property methods.

The properties month and day use a less readable form of checking the data type. Replacing not self.dtype == "datetime64[Y]" with self.dtype != "datetime64[Y]" will improve the readability and maintainability of the code.

Apply this diff to refine the logical expression:

-        if not self.dtype == "datetime64[Y]":
+        if self.dtype != "datetime64[Y]":
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@property
def year(self):
return int(str(self.astype("datetime64[Y]")))
@property
def month(self):
# if date unit is year, don't return a month (only M/D)
if not self.dtype == "datetime64[Y]":
return int(str(self.astype("datetime64[M]")).split("-")[-1])
@property
def day(self):
# only return a day if date unit is in days
if self.dtype == "datetime64[D]":
return int(str(self.astype("datetime64[D]")).split("-")[-1])
@property
def year(self):
return int(str(self.astype("datetime64[Y]")))
@property
def month(self):
# if date unit is year, don't return a month (only M/D)
if self.dtype != "datetime64[Y]":
return int(str(self.astype("datetime64[M]")).split("-")[-1])
@property
def day(self):
# only return a day if date unit is in days
if self.dtype == "datetime64[D]":
return int(str(self.astype("datetime64[D]")).split("-")[-1])
Tools
Ruff

68-68: Use self.dtype != "datetime64[Y]" instead of not self.dtype == "datetime64[Y]"

Replace with != operator

(SIM201)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error_handling_issue
The day property also lacks a return statement if the condition is not met. Similar to the month property, consider raising an exception or returning a default value to avoid returning None implicitly.



class DatePrecision(IntEnum):
"""date precision, to indicate date precision independent from how much
of the date is known."""

# numbers should be set to allow logical greater than / less than
# comparison, e.g. year precision > month

#: day
DAY = 1
#: month
MONTH = 2
#: year
YEAR = 3

def __str__(self):
return f"{self.name}"

# NOTE: consider harmonizing / using numpy date units:
# years (‘Y’), months (‘M’), weeks (‘W’), and days (‘D’)
13 changes: 11 additions & 2 deletions src/undate/dateformat/iso8601.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,17 @@ def to_string(self, undate: Undate) -> str:
# and not others; force year to always be 4 digits
if date_portion == "year":
date_parts.append("%04d" % undate.earliest.year)
else:
date_parts.append(undate.earliest.strftime(iso_format))
elif date_portion == "month":
date_parts.append("%02d" % undate.earliest.month)
elif date_portion == "day":
date_parts.append("%02d" % undate.earliest.day)

Comment on lines +64 to +68
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enhanced date portion handling in to_string method.

The modifications to explicitly handle "month" and "day" portions improve the method's ability to format dates accurately. The use of string formatting ensures that these portions are always represented with two digits, which enhances consistency.

However, the removal of the previously commented-out code (lines 69-74) reduces the method's flexibility to handle other date portions dynamically. If flexibility in handling various date formats is a requirement, consider reintroducing or refactoring this part of the code.

# else:
# # date_parts.append(undate.earliest.strftime(iso_format))
# e = undate.earliest
# # isoformat defined above per field
# date_parts.append(f"{e.year:04d}") # -{e.month:02d}-{e.day:02d}")
# date_parts.append(undate.earliest.strftime(iso_format))
elif date_portion == "year":
# if not known but this is year, add '-' for --MM-DD unknown year format
date_parts.append("-")
Expand Down
88 changes: 37 additions & 51 deletions src/undate/undate.py
Original file line number Diff line number Diff line change
@@ -1,48 +1,27 @@
import datetime
from calendar import monthrange
from enum import IntEnum
import re
from calendar import monthrange

# Pre 3.10 requires Union for multiple types, e.g. Union[int, None] instead of int | None
from typing import Optional, Dict, Union
from typing import Optional, Dict, Union, Any
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused import.

The import for typing.Any is flagged as unused. Removing unused imports helps maintain the cleanliness and readability of the code.

Proposed removal:

-from typing import Optional, Dict, Union, Any
+from typing import Optional, Dict, Union
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
from typing import Optional, Dict, Union, Any
from typing import Optional, Dict, Union
Tools
Ruff

6-6: typing.Any imported but unused

Remove unused import: typing.Any

(F401)


from dateutil.relativedelta import relativedelta
import numpy as np
from numpy.typing import ArrayLike, DTypeLike
Comment on lines +8 to +9
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove unused imports.

The imports for numpy, ArrayLike, and DTypeLike are flagged as unused by static analysis tools. If these are not used in the current implementation, they should be removed to clean up the code.

Proposed removal:

-import numpy as np
-from numpy.typing import ArrayLike, DTypeLike
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import numpy as np
from numpy.typing import ArrayLike, DTypeLike
Tools
Ruff

8-8: numpy imported but unused

Remove unused import: numpy

(F401)


9-9: numpy.typing.ArrayLike imported but unused

Remove unused import

(F401)


9-9: numpy.typing.DTypeLike imported but unused

Remove unused import

(F401)


from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX
from undate.dateformat.base import BaseDateFormat


#: duration of a single day
ONE_DAY = datetime.timedelta(days=1)


class DatePrecision(IntEnum):
"""date precision, to indicate date precision independent from how much
of the date is known."""

# numbers should be set to allow logical greater than / less than
# comparison, e.g. year precision > month

#: day
DAY = 1
#: month
MONTH = 2
#: year
YEAR = 3

def __str__(self):
return f"{self.name}"


class Undate:
"""Simple object for representing uncertain, fuzzy or partially unknown dates"""
"""object for representing uncertain, fuzzy or partially unknown dates"""

DEFAULT_FORMAT: str = "ISO8601"

#: symbol for unknown digits within a date value
MISSING_DIGIT: str = "X"

earliest: datetime.date
latest: datetime.date
earliest: Date
latest: Date
#: A string to label a specific undate, e.g. "German Unity Date 2022" for Oct. 3, 2022.
#: Labels are not taken into account when comparing undate objects.
label: Union[str, None] = None
Expand Down Expand Up @@ -88,14 +67,17 @@ def __init__(
min_year = int(str(year).replace(self.MISSING_DIGIT, "0"))
max_year = int(str(year).replace(self.MISSING_DIGIT, "9"))
else:
min_year = datetime.MINYEAR
max_year = datetime.MAXYEAR
# numpy datetime is stored as 64-bit integer, so min/max
# depends on the time unit; assume days for now
# See https://numpy.org/doc/stable/reference/arrays.datetime.html#datetime-units
max_year = int(2.5e16)
min_year = int(-2.5e16)
Comment on lines +70 to +74
Copy link

@coderabbitai coderabbitai bot Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider implications of using extreme values for min_year and max_year.

The use of 2.5e16 for min_year and max_year is based on the maximum range of numpy.datetime64. However, this might introduce practical issues or inefficiencies in date handling, especially if the application does not require such an extensive range. Consider whether a more practical default range could be used, or if this should be configurable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that I agree with the suggestion that this be configurable or what the advantages of that would be, if we're initializing a 64bit int I don't think making the min/max smaller would save any memory. If we're committing to NumPy then we should use it consistently.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ColeDCrawford, you make a good point about the consistency with NumPy and the lack of memory savings by reducing the range. I agree that if the project is committed to using NumPy, it makes sense to leverage its full capabilities without introducing unnecessary configurability. Thanks for the clarification!


Failed to remove learnings

Learnings cannot be removed due to server error. Please try again.


# if month is passed in as a string but completely unknown,
# treat as none
# TODO: we should preserve this information somehow;
# difference between just a year and and an unknown month within a year
# maybe in terms of granularity / size ?
# maybe in terms of date precision ?
if month == "XX":
month = None

Expand Down Expand Up @@ -143,8 +125,8 @@ def __init__(

# for unknowns, assume smallest possible value for earliest and
# largest valid for latest
self.earliest = datetime.date(min_year, min_month, min_day)
self.latest = datetime.date(max_year, max_month, max_day)
self.earliest = Date(min_year, min_month, min_day)
self.latest = Date(max_year, max_month, max_day)

if formatter is None:
# import all subclass definitions; initialize the default
Expand Down Expand Up @@ -261,7 +243,7 @@ def __gt__(self, other: object) -> bool:
# strictly greater than must rule out equals
return not (self < other or self == other)

def __le__(self, other: Union["Undate", datetime.date]) -> bool:
def __le__(self, other: object) -> bool:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplify comparison logic.

The __le__ method uses a straightforward comparison logic that could potentially be simplified or optimized. Review whether the current implementation aligns with the intended logic, especially in handling edge cases or unusual comparisons.

return self == other or self < other

def __contains__(self, other: object) -> bool:
Expand All @@ -272,15 +254,17 @@ def __contains__(self, other: object) -> bool:
if self == other:
return False

return (
self.earliest <= other.earliest
and self.latest >= other.latest
# is precision sufficient for comparing partially known dates?
and self.precision > other.precision
return all(
[
self.earliest <= other.earliest,
self.latest >= other.latest,
# is precision sufficient for comparing partially known dates?
self.precision > other.precision,
]
Comment on lines +257 to +263
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review precision comparison in __contains__ method.

The comparison of self.precision > other.precision in the __contains__ method assumes a certain hierarchy or ordering of precision values. This logic might not be intuitive or could lead to incorrect results if the precision levels are not strictly comparable in a linear fashion.

)

@staticmethod
def from_datetime_date(dt_date):
def from_datetime_date(dt_date: datetime.date):
"""Initialize an :class:`Undate` object from a :class:`datetime.date`"""
return Undate(dt_date.year, dt_date.month, dt_date.day)

Expand All @@ -300,7 +284,7 @@ def is_known(self, part: str) -> bool:
def is_partially_known(self, part: str) -> bool:
return isinstance(self.initial_values[part], str)

def duration(self) -> datetime.timedelta:
def duration(self): # -> np.timedelta64:
"""What is the duration of this date?
Calculate based on earliest and latest date within range,
taking into account the precision of the date even if not all
Expand All @@ -318,16 +302,18 @@ def duration(self) -> datetime.timedelta:
if not self.known_year:
# if year is unknown, calculate month duration in
# a single year
latest = datetime.date(
self.earliest.year, self.latest.month, self.latest.day
)
latest = Date(self.earliest.year, self.latest.month, self.latest.day)

# latest = datetime.date(
# self.earliest.year, self.latest.month, self.latest.day
# )
delta = latest - self.earliest + ONE_DAY
# month duration can't ever be more than 31 days
# (could we ever know if it's smaller?)

# if granularity == month but not known month, duration = 31
if delta.days > 31:
return datetime.timedelta(days=31)
if delta.astype(int) > 31:
return ONE_MONTH_MAX
return delta

# otherwise, calculate based on earliest/latest range
Expand Down Expand Up @@ -407,11 +393,11 @@ def __eq__(self, other) -> bool:
# consider interval equal if both dates are equal
return self.earliest == other.earliest and self.latest == other.latest

def duration(self) -> datetime.timedelta:
def duration(self): # -> np.timedelta64:
"""Calculate the duration between two undates.

:returns: A duration
:rtype: timedelta
:rtype: numpy.timedelta64
"""
# what is the duration of this date range?

Expand All @@ -431,8 +417,8 @@ def duration(self) -> datetime.timedelta:
# if we get a negative, we've wrapped from end of one year
# to the beginning of the next;
# recalculate assuming second date is in the subsequent year
if duration.days < 0:
end = self.latest.earliest + relativedelta(years=1)
if duration.astype("int") < 0:
end = self.latest.earliest + ONE_YEAR
duration = end - self.earliest.earliest

# add the additional day *after* checking for a negative
Expand Down
43 changes: 43 additions & 0 deletions tests/test_date.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import numpy as np

from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX
Comment on lines +1 to +3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Unused Imports

The static analysis has correctly identified several unused imports:

  • numpy is imported but not used directly in the tests.
  • ONE_DAY, ONE_YEAR, and ONE_MONTH_MAX are imported but not used.

Consider removing these to clean up the code and reduce confusion about dependencies.

Apply this diff to remove the unused imports:

-import numpy as np
-from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX
+from undate.date import Date, DatePrecision
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import numpy as np
from undate.date import Date, DatePrecision, ONE_DAY, ONE_YEAR, ONE_MONTH_MAX
from undate.date import Date, DatePrecision
Tools
Ruff

1-1: numpy imported but unused

Remove unused import: numpy

(F401)


3-3: undate.date.ONE_DAY imported but unused

Remove unused import

(F401)


3-3: undate.date.ONE_YEAR imported but unused

Remove unused import

(F401)


3-3: undate.date.ONE_MONTH_MAX imported but unused

Remove unused import

(F401)



class TestDatePrecision:
def test_str(self):
assert str(DatePrecision.YEAR) == "YEAR"


class TestDate:
def test_init_year(self):
d = Date(2001)
assert isinstance(d, Date)
assert d.dtype == "datetime64[Y]"
assert str(d) == "2001"

def test_init_year_month(self):
d = Date(2010, 5)
assert isinstance(d, Date)
assert d.dtype == "datetime64[M]"
assert str(d) == "2010-05"

def test_init_year_month(self):
Copy link

@coderabbitai coderabbitai bot Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct Redefinition of Test Function

The function test_init_year_month is defined twice (lines 18 and 24), which is likely an oversight. This redefinition could cause one of the tests to be skipped, leading to incomplete test coverage.

Consider renaming one of the functions or combining them if they are intended to test different scenarios. Here is a proposed fix to rename the second function to test_init_year_month_day:

-    def test_init_year_month(self):
+    def test_init_year_month_day(self):

Also applies to: 18-18

Tools
Ruff

24-24: Redefinition of unused test_init_year_month from line 18

(F811)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, I think I found and fixed this in a different branch when I was working on it yesterday

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rlskoeser, thanks for the update! Since you've already fixed this in another branch, no further action is needed here regarding the duplicate test function. I'll proceed with the rest of the review.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

d = Date(2021, 6, 15)
assert isinstance(d, Date)
assert d.dtype == "datetime64[D]"
assert str(d) == "2021-06-15"

def test_properties_year(self):
assert Date(2001).year == 2001
assert Date(2010, 5).year == 2010
assert Date(2021, 6, 15).year == 2021

def test_properties_month(self):
assert Date(2001).month is None
assert Date(2010, 5).month == 5
assert Date(2021, 6, 15).month == 6

def test_properties_day(self):
assert Date(2001).day is None
assert Date(2010, 5).day == None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use is for None Comparisons

The comparison to None using == is not recommended. Using is is the correct practice in Python for clarity and correctness.

Apply this diff to correct the comparison:

-        assert Date(2010, 5).day == None
+        assert Date(2010, 5).day is None
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert Date(2010, 5).day == None
assert Date(2010, 5).day is None
Tools
Ruff

42-42: Comparison to None should be cond is None

Replace with cond is None

(E711)

assert Date(2021, 6, 15).day == 15
Loading
Loading