Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use numpy datetime64 instead of datetime.date #77 #84
Changes from 7 commits
436431b
220dc7e
3306bde
0396a64
1a75640
329fa3d
282432e
616ae2f
2a7de40
2327d7b
039ff0e
8a4b67e
ed3f493
e197cb2
61be76f
e494d7a
0806dc9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
andUnion
from thetyping
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:
Committable suggestion
Tools
Ruff
There was a problem hiding this comment.
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 isdatetime64
a subclass ofint
?There was a problem hiding this comment.
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 :)There was a problem hiding this comment.
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 genericException
for error handling. It would be more informative to use a specific exception type, such asValueError
, to provide clearer feedback to the user about what went wrong.Consider refining the exception handling with a more specific exception type:
Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want the properties to have return types?
There was a problem hiding this comment.
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
andday
use a less readable form of checking the data type. Replacingnot self.dtype == "datetime64[Y]"
withself.dtype != "datetime64[Y]"
will improve the readability and maintainability of the code.Apply this diff to refine the logical expression:
Committable suggestion
Tools
Ruff
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Committable suggestion
Tools
Ruff
There was a problem hiding this comment.
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
, andDTypeLike
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:
Committable suggestion
Tools
Ruff
There was a problem hiding this comment.
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
andmax_year
.The use of
2.5e16
formin_year
andmax_year
is based on the maximum range ofnumpy.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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks!
There was a problem hiding this comment.
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
, andONE_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:
Committable suggestion
Tools
Ruff
There was a problem hiding this comment.
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
:Also applies to: 18-18
Tools
Ruff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use
is
for None ComparisonsThe comparison to
None
using==
is not recommended. Usingis
is the correct practice in Python for clarity and correctness.Apply this diff to correct the comparison:
Committable suggestion
Tools
Ruff