Conversation
…r Python versions
* Bump pypa/gh-action-pypi-publish from 1.9.0 to 1.10.0 (#27) Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.9.0 to 1.10.0. - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@v1.9.0...v1.10.0) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump pypa/gh-action-pypi-publish from 1.10.0 to 1.10.1 (#28) Bumps [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish) from 1.10.0 to 1.10.1. - [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases) - [Commits](pypa/gh-action-pypi-publish@v1.10.0...v1.10.1) --- updated-dependencies: - dependency-name: pypa/gh-action-pypi-publish dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
Reviewer's GuideThis PR implements v1.3.0 by extending TemporalAdjuster with new absolute date and time adjusters, introduces ExtendedTimeDelta for month/year support, enhances weekday operations with occurrence queries and modern type hints, refines the sequenceable decorator, and updates project tooling and configurations for documentation, linting, and CI/CD. Class diagram for new and updated date/time adjustersclassDiagram
class TemporalAdjuster {
<<inherits>>
}
class _AbsoluteDateOperations {
+int_to_day_of_year(date, int_value) DateT
+int_to_day_of_month(date, int_value) DateT
+date_to_int_of_year(date) int
+date_to_int_of_month(date) int
}
class _TimeAdjuster {
+time_difference(time_obj_1, time_obj_2) ExtendedTimeDelta
+is_time_in_range(time_obj, start, end) bool
+round_time(time_obj, round_to) TimeT
+time_to_seconds(time_obj) float
+seconds_to_time(seconds) time
}
class _TemporalAdjusterForWeekday {
+which_of_month(weekday, date) int
+which_of_year(weekday, date) int
...
}
class ExtendedTimeDelta {
+months: int
+years: int
+to_timedelta() timedelta
+to_microseconds() float
+to_seconds() float
+to_minutes() float
+to_hours() float
+to_days() float
+to_weeks() float
+to_months() float
+to_years() float
}
TemporalAdjuster <|-- _AbsoluteDateOperations
TemporalAdjuster <|-- _TemporalAdjusterForFirstAndLastDays
TemporalAdjuster <|-- _TemporalAdjusterForWeekday
TemporalAdjuster <|-- _TimeAdjuster
_TimeAdjuster ..> ExtendedTimeDelta : uses
_AbsoluteDateOperations <.. TemporalAdjuster : inherited
_TemporalAdjusterForWeekday <.. TemporalAdjuster : inherited
_TimeAdjuster <.. TemporalAdjuster : inherited
ExtendedTimeDelta <|-- timedelta
Class diagram for the updated sequenceable decoratorclassDiagram
class sequenceable {
+__call__(func: Callable[P, R]) -> Callable[P, R | Any]
}
sequenceable o-- Callable
sequenceable o-- numpy.ndarray
Class diagram for new and updated date/time type aliasesclassDiagram
class AnyDate
class DateT
class AnyTime
class TimeT
AnyDate <|-- datetime
AnyDate <|-- date
AnyTime <|-- datetime
AnyTime <|-- time
DateT <|-- AnyDate
TimeT <|-- AnyTime
Class diagram for ExtendedTimeDeltaclassDiagram
class ExtendedTimeDelta {
+months: int
+years: int
+from_timedelta(td: timedelta) ExtendedTimeDelta
+to_timedelta() timedelta
+to_microseconds() float
+to_seconds() float
+to_minutes() float
+to_hours() float
+to_days() float
+to_weeks() float
+to_months() float
+to_years() float
}
ExtendedTimeDelta <|-- timedelta
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Pull Request Overview
This PR focuses on updating the Temporal Adjuster project to version 1.3.0, adding significant new functionality while maintaining code quality and consistency. The changes primarily involve adding time operations, absolute date operations, enhanced types, and improving code formatting across the codebase.
- Added new time operations module with methods for time calculations, rounding, and range checks
- Added absolute date operations for converting between dates and integer representations
- Enhanced the ExtendedTimeDelta class with comprehensive date/time duration support
- Updated formatting and linting configuration with more comprehensive rules
Reviewed Changes
Copilot reviewed 53 out of 56 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_*.py | Added comprehensive test coverage for new functionality while updating formatting |
| temporal_adjuster/temporal_adjuster.py | Integrated new module classes into main TemporalAdjuster class |
| temporal_adjuster/modules/ | Added new time_operations and absolute_date_operations modules |
| temporal_adjuster/common/types/ | Enhanced type system with ExtendedTimeDelta and additional time types |
| docs/ | Updated documentation structure and configuration |
| ruff.toml | Modernized linting configuration with stricter rules |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+5
to
+11
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: chartboost/ruff-action@v1 | ||
| with: | ||
| args: "format --check" | ||
| coverage: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the problem, we need to add a permissions block setting explicit, least-privilege permissions for the jobs that currently have no such block: specifically, add this at the job level (ruff, coverage, and security) or at the workflow root to apply by default to all jobs. The best and simplest fix is to add a top-level permissions block to the workflow, immediately after the workflow name and event trigger block, setting contents: read. This will make all jobs default to only read access (except where overridden, like scan), meeting the CodeQL recommendation and GitHub security best practices. No changes to individual job internals or steps are required. If later, a job needs more permissions, add or override as needed.
Specifically:
- In
.github/workflows/package_quality.yml, insert the following block after thename:and beforeon:or directly afteron: ...:permissions: contents: read
This will apply least-privilege permissions globally, except for jobs with an explicit block (scan).
No new imports, methods, or definitions are needed outside of this YAML edit.
Suggested changeset
1
.github/workflows/package_quality.yml
| @@ -1,4 +1,6 @@ | ||
| name: Package quality | ||
| permissions: | ||
| contents: read | ||
| on: [push, pull_request] | ||
| jobs: | ||
| ruff: |
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines
+12
to
+42
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Set up Python | ||
| uses: actions/setup-python@v5.2.0 | ||
| with: | ||
| python-version: 3.12 | ||
| - name: Install dependencies | ||
| run: | | ||
| python -m pip install --upgrade pip | ||
| python -m pip install coverage | ||
| pip install -r requirements.dev.txt | ||
| - name: Check coverage threshold | ||
| run: | | ||
| coverage run -m unittest discover tests/ -v | ||
| coverage report --fail-under=95 | ||
|
|
||
| - name: Upload coverage reports to Codecov | ||
| uses: codecov/codecov-action@v4.0.1 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
|
|
||
| - name: Check type completeness | ||
| uses: gtkacz/pyanalyze-action@v1 | ||
|
|
||
| - name: Check for dead code | ||
| uses: gtkacz/vulture-action@1.0.0 | ||
| with: | ||
| args: '--min-confidence 70 --exclude "*/docs/*,setup.py"' | ||
|
|
||
| security: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To resolve the problem, add an explicit permissions block limiting GITHUB_TOKEN permissions to the least privilege required. Since there is already a permissions block within the scan job, the best approach is to set a root-level permissions block so that all jobs get limited permissions by default, and specific jobs can override if needed. Based on best practices and the provided recommendation, add the following block at the top level (below name but above on):
permissions:
contents: readThis ensures that all jobs in the workflow inherit contents: read as the minimal required permission for accessing repository content, unless they explicitly specify different permissions (as scan does). No further methods, imports, or definitions are needed.
Suggested changeset
1
.github/workflows/package_quality.yml
| @@ -1,4 +1,6 @@ | ||
| name: Package quality | ||
| permissions: | ||
| contents: read | ||
| on: [push, pull_request] | ||
| jobs: | ||
| ruff: |
Copilot is powered by AI and may make mistakes. Always verify output.
Comment on lines
+43
to
+58
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@master | ||
| - name: Run Snyk to check for vulnerabilities | ||
| uses: snyk/actions/python-3.10@master | ||
| continue-on-error: true | ||
| env: | ||
| SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }} | ||
| with: | ||
| args: --sarif-file-output=snyk.sarif | ||
| - name: Upload result to GitHub Code Scanning | ||
| uses: github/codeql-action/upload-sarif@v2 | ||
| with: | ||
| sarif_file: snyk.sarif | ||
|
|
||
| scan: |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, an explicit permissions block should be added. The best way is to set permissions: contents: read at the workflow root (before the jobs: section) to restrict all jobs to read-only permissions unless otherwise specified. This enforces the principle of least privilege. If a job requires additional permissions, a job-specific block should be added (as already done for the scan job). The change should be made at the very top of the file after the name: and before the on: or jobs: keys to ensure it applies to the workflow as a whole wherever not overridden.
Suggested changeset
1
.github/workflows/package_quality.yml
| @@ -1,4 +1,6 @@ | ||
| name: Package quality | ||
| permissions: | ||
| contents: read | ||
| on: [push, pull_request] | ||
| jobs: | ||
| ruff: |
Copilot is powered by AI and may make mistakes. Always verify output.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @gtkacz - I've reviewed your changes - here's some feedback:
- Add validation in int_to_day_of_year and int_to_day_of_month to ensure the provided index is within the valid range for that year or month and raise a clear error when out of bounds.
- The which_of_month and which_of_year methods take a weekday argument but never use it to verify that the date’s weekday matches; either enforce that check or remove the unused parameter.
- In the sequenceable decorator, converting every iterable to a NumPy array can break on non–numeric or custom sequence types—consider falling back to plain Python iteration when NumPy conversion isn’t appropriate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add validation in int_to_day_of_year and int_to_day_of_month to ensure the provided index is within the valid range for that year or month and raise a clear error when out of bounds.
- The which_of_month and which_of_year methods take a weekday argument but never use it to verify that the date’s weekday matches; either enforce that check or remove the unused parameter.
- In the sequenceable decorator, converting every iterable to a NumPy array can break on non–numeric or custom sequence types—consider falling back to plain Python iteration when NumPy conversion isn’t appropriate.
## Individual Comments
### Comment 1
<location> `temporal_adjuster/common/types/__extended_timedelta.py:6` </location>
<code_context>
+from typing import Self, Union
+
+
+class ExtendedTimeDelta(timedelta):
+ """
+ An extended version of Python's timedelta that supports months and years.
</code_context>
<issue_to_address>
Subclassing timedelta may lead to subtle issues due to its C implementation.
Subclassing timedelta can cause unexpected behavior, especially when adding new fields like months and years. Consider using composition instead, or clearly document the limitations and test for edge cases.
</issue_to_address>
### Comment 2
<location> `temporal_adjuster/common/types/__extended_timedelta.py:16` </location>
<code_context>
+ are approximations and may not be suitable for all use cases.
+ """
+
+ __slots__ = (
+ '_days',
+ '_hashcode',
+ '_microseconds',
+ '_months',
+ '_seconds',
+ '_years',
+ )
+
</code_context>
<issue_to_address>
Defining __slots__ may not be compatible with all uses of timedelta subclassing.
Subclasses of C extension types like timedelta may not safely support __slots__. Consider removing __slots__ or verify all parent classes are compatible to avoid potential bugs or crashes.
</issue_to_address>
### Comment 3
<location> `temporal_adjuster/common/types/__extended_timedelta.py:67` </location>
<code_context>
+ 1 month, 15 days, 0:00:00
+
+ """
+ cls.DAYS_IN_MONTH = days_in_month
+ cls.DAYS_IN_YEAR = days_in_year
+
+ # Process microseconds
</code_context>
<issue_to_address>
Assigning DAYS_IN_MONTH and DAYS_IN_YEAR as class variables can cause thread-safety issues.
Since these variables are shared across all instances, concurrent modifications can cause incorrect behavior. Use instance variables or immutable class constants instead.
</issue_to_address>
### Comment 4
<location> `temporal_adjuster/common/types/__extended_timedelta.py:232` </location>
<code_context>
+ parent_result = parent_self - other
+ return ExtendedTimeDelta.from_timedelta(parent_result)
+
+ def __mul__(self, other: Union[timedelta, 'ExtendedTimeDelta']) -> Self:
+ """
+ Multiply this ExtendedTimeDelta by an integer.
</code_context>
<issue_to_address>
The __mul__ method signature and implementation do not match typical timedelta behavior.
Update the type hint and docstring to indicate that __mul__ should accept a number (int or float), not a timedelta. The implementation should raise a TypeError if 'other' is not numeric. Also, implement __rmul__ for commutative support.
</issue_to_address>
### Comment 5
<location> `temporal_adjuster/common/types/__extended_timedelta.py:497` </location>
<code_context>
+ ),
+ )
+
+ def __dict__(self) -> dict:
+ """
+ Return a dictionary representation of the ExtendedTimeDelta.
</code_context>
<issue_to_address>
Overriding __dict__ as a method is non-standard and may break introspection.
__dict__ should remain an attribute; using it as a method can cause compatibility issues. Please rename the method to something like to_dict.
</issue_to_address>
### Comment 6
<location> `temporal_adjuster/common/types/__extended_timedelta.py:367` </location>
<code_context>
+ if isinstance(other, (ExtendedTimeDelta, timedelta)):
+ return self._cmp(other) >= 0
+
+ def _cmp(self, other: Union[timedelta, 'ExtendedTimeDelta']) -> int:
+ """
+ Compare this ExtendedTimeDelta with another time delta.
</code_context>
<issue_to_address>
The _cmp method returns a timedelta or int, which may not be consistent.
Currently, comparing with a standard timedelta returns a timedelta object instead of an int, which may break comparison operators. To ensure consistency, always return an int—consider using total_seconds or another scalar for comparison.
</issue_to_address>
### Comment 7
<location> `temporal_adjuster/modules/time_operations.py:64` </location>
<code_context>
+ if isinstance(time_obj, datetime):
+ time_obj = time_obj.time()
+
+ return start <= time_obj <= end if start <= end else time_obj >= start or time_obj <= end
+
+ @staticmethod
</code_context>
<issue_to_address>
The time range check may not handle all edge cases with equal start and end times.
When start == end, the function returns True for any time, which may not be intended. Please clarify, document, or handle this edge case explicitly.
Suggested implementation:
```python
if isinstance(time_obj, datetime):
time_obj = time_obj.time()
# Handle the edge case where start == end
# If start == end, only return True if time_obj == start (i.e., the range is a single point)
if start == end:
return time_obj == start
return start <= time_obj <= end if start < end else time_obj >= start or time_obj <= end
@staticmethod
```
```python
"""
Calculate if a time object falls within a time range, accounting for wrapping around midnight.
Edge case:
- If start == end, the range is considered a single point in time, and only time_obj == start returns True.
"""
```
</issue_to_address>
### Comment 8
<location> `temporal_adjuster/modules/time_operations.py:110` </location>
<code_context>
+ float: The total number of seconds since midnight.
+
+ """
+ return time_obj.hour * 3600 + time_obj.minute * 60 + time_obj.second + time_obj.microsecond / 1e6
+
+ @staticmethod
</code_context>
<issue_to_address>
No type check for time_obj in time_to_seconds may cause AttributeError.
Add a type check or input validation to prevent AttributeError when time_obj is not a time or datetime instance.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
"""
Convert a time object to the total number of seconds since midnight.
Args:
time_obj (AnyTime): The time object to convert.
Returns:
float: The total number of seconds since midnight.
"""
return time_obj.hour * 3600 + time_obj.minute * 60 + time_obj.second + time_obj.microsecond / 1e6
=======
"""
Convert a time object to the total number of seconds since midnight.
Args:
time_obj (AnyTime): The time object to convert.
Returns:
float: The total number of seconds since midnight.
"""
import datetime
if not isinstance(time_obj, (datetime.time, datetime.datetime)):
raise TypeError(
f"time_obj must be an instance of datetime.time or datetime.datetime, got {type(time_obj)}"
)
return time_obj.hour * 3600 + time_obj.minute * 60 + time_obj.second + time_obj.microsecond / 1e6
>>>>>>> REPLACE
</suggested_fix>
### Comment 9
<location> `temporal_adjuster/modules/time_operations.py:131` </location>
<code_context>
+ minute = int(seconds // 60)
+ seconds %= 60
+
+ return time(hour, minute, int(seconds), int((seconds - int(seconds)) * 1e6))
</code_context>
<issue_to_address>
Possible floating point rounding issues in seconds_to_time.
The microseconds calculation can yield inaccurate or negative values due to floating point errors. Use round() or clamp the result to [0, 999999] to ensure correctness.
</issue_to_address>
### Comment 10
<location> `tests/test_absolute_date_operations.py:145` </location>
<code_context>
+
+ def test_edge_cases(self):
+ """Test edge cases and potential error conditions."""
+ result = TemporalAdjuster.int_to_day_of_year(date(2020, 1, 1), 60)
+ self.assertEqual(result, date(2020, 2, 29))
+
+ result = TemporalAdjuster.date_to_int_of_year([])
+ self.assertEqual(result, [])
+
+ result = TemporalAdjuster.date_to_int_of_month([date(2021, 5, 20)])
+ self.assertEqual(result, [20])
</code_context>
<issue_to_address>
Consider adding tests for invalid or out-of-range day numbers.
Please add tests for invalid day numbers (e.g., 0, negative, or exceeding valid range) to verify proper error handling in these methods.
</issue_to_address>
### Comment 11
<location> `tests/test_extended_timedelta.py:45` </location>
<code_context>
+ self.assertEqual(et.days, 1)
+ self.assertEqual(et.seconds, 43200)
+
+ def test_initialization_negative_days(self):
+ et = ExtendedTimeDelta(days=-5, days_in_month=30)
+ self.assertEqual(et.years, -1)
+ self.assertEqual(et.months, 11)
+ self.assertEqual(et.days, 25)
+
+ def test_properties(self):
+ et = ExtendedTimeDelta(years=3, months=4, days=5, seconds=6, microseconds=7)
+ self.assertEqual(et.years, 3)
</code_context>
<issue_to_address>
Consider adding tests for invalid or extreme values in ExtendedTimeDelta.
Please add tests for negative, very large, and non-integer values, as well as for conflicting or ambiguous arguments, to ensure robust handling of all input scenarios.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_properties(self):
et = ExtendedTimeDelta(years=3, months=4, days=5, seconds=6, microseconds=7)
self.assertEqual(et.years, 3)
self.assertEqual(et.months, 4)
self.assertEqual(et.days, 5)
self.assertEqual(et.seconds, 6)
self.assertEqual(et.microseconds, 7)
=======
def test_properties(self):
et = ExtendedTimeDelta(years=3, months=4, days=5, seconds=6, microseconds=7)
self.assertEqual(et.years, 3)
self.assertEqual(et.months, 4)
self.assertEqual(et.days, 5)
self.assertEqual(et.seconds, 6)
self.assertEqual(et.microseconds, 7)
def test_invalid_non_integer_values(self):
with self.assertRaises(TypeError):
ExtendedTimeDelta(years="one")
with self.assertRaises(TypeError):
ExtendedTimeDelta(months=[1, 2])
with self.assertRaises(TypeError):
ExtendedTimeDelta(days={"days": 1})
with self.assertRaises(TypeError):
ExtendedTimeDelta(seconds=None)
with self.assertRaises(TypeError):
ExtendedTimeDelta(microseconds="1000")
def test_large_values(self):
et = ExtendedTimeDelta(years=10**6, months=10**6, days=10**6, seconds=10**6, microseconds=10**6)
self.assertEqual(et.years, 10**6)
self.assertEqual(et.months, 10**6)
self.assertEqual(et.days, 10**6)
self.assertEqual(et.seconds, 10**6)
self.assertEqual(et.microseconds, 10**6)
def test_conflicting_arguments(self):
# Assuming ExtendedTimeDelta should handle negative and positive values together
et = ExtendedTimeDelta(years=1, months=-2, days=30, seconds=-60)
self.assertEqual(et.years, 1)
self.assertEqual(et.months, -2)
self.assertEqual(et.days, 30)
self.assertEqual(et.seconds, -60)
def test_non_integer_values(self):
et = ExtendedTimeDelta(years=1.5, months=2.7, days=3.9, seconds=4.1, microseconds=5.6)
self.assertEqual(et.years, 1)
self.assertEqual(et.months, 2)
self.assertEqual(et.days, 3)
self.assertEqual(et.seconds, 4)
self.assertEqual(et.microseconds, 5)
>>>>>>> REPLACE
</suggested_fix>
### Comment 12
<location> `CHANGELOG.md:15` </location>
<code_context>
+### Added
+
+- New `day_of_year` method, that returns an integer ranging from 1-365 (or 366 for leap years) corresponding to the day of the year.
+- New `which_of_month` and `which_of_year` weekday methods, that return an integer representing which occurence of a weekday in a month or year a given date falls on. For example, `which_of_month` will return 1 for the first Monday of the month, 2 for the second Monday, and so on. Similarly, `which_of_year` will return 1 for the first Monday of the year, 2 for the second Monday, and so on.
+- Altered sequence processing to make use of numpy vectorization.
+
</code_context>
<issue_to_address>
Typo: 'occurence' should be 'occurrence'.
Update the changelog to use the correct spelling: 'occurrence'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- New `which_of_month` and `which_of_year` weekday methods, that return an integer representing which occurence of a weekday in a month or year a given date falls on. For example, `which_of_month` will return 1 for the first Monday of the month, 2 for the second Monday, and so on. Similarly, `which_of_year` will return 1 for the first Monday of the year, 2 for the second Monday, and so on.
=======
- New `which_of_month` and `which_of_year` weekday methods, that return an integer representing which occurrence of a weekday in a month or year a given date falls on. For example, `which_of_month` will return 1 for the first Monday of the month, 2 for the second Monday, and so on. Similarly, `which_of_year` will return 1 for the first Monday of the year, 2 for the second Monday, and so on.
>>>>>>> REPLACE
</suggested_fix>
### Comment 13
<location> `temporal_adjuster/common/decorators/sequence_processor.py:17` </location>
<code_context>
-def sequenceable(target: str):
+def sequenceable(target: str) -> Callable[[Callable[P, R]], Callable[P, R | Any]]:
"""
- This decorator is used to process if a sequence of values passed as an argument to a function. The function is called for each value in the sequence, and the result is stored in the same position in the sequence.
</code_context>
<issue_to_address>
The return type annotation for the decorator is too broad and may reduce type safety and clarity.
The return type annotation `Callable[P, R | Any]` is overly broad and may not accurately reflect the actual return type, potentially reducing type safety and maintainability. Consider using a more precise type or a type variable to better capture the decorator's behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| from typing import Self, Union | ||
|
|
||
|
|
||
| class ExtendedTimeDelta(timedelta): |
There was a problem hiding this comment.
issue (bug_risk): Subclassing timedelta may lead to subtle issues due to its C implementation.
Subclassing timedelta can cause unexpected behavior, especially when adding new fields like months and years. Consider using composition instead, or clearly document the limitations and test for edge cases.
Comment on lines
+16
to
+22
| __slots__ = ( | ||
| '_days', | ||
| '_hashcode', | ||
| '_microseconds', | ||
| '_months', | ||
| '_seconds', | ||
| '_years', |
There was a problem hiding this comment.
issue (bug_risk): Defining slots may not be compatible with all uses of timedelta subclassing.
Subclasses of C extension types like timedelta may not safely support slots. Consider removing slots or verify all parent classes are compatible to avoid potential bugs or crashes.
Comment on lines
+67
to
+68
| cls.DAYS_IN_MONTH = days_in_month | ||
| cls.DAYS_IN_YEAR = days_in_year |
There was a problem hiding this comment.
issue (bug_risk): Assigning DAYS_IN_MONTH and DAYS_IN_YEAR as class variables can cause thread-safety issues.
Since these variables are shared across all instances, concurrent modifications can cause incorrect behavior. Use instance variables or immutable class constants instead.
| parent_result = parent_self - other | ||
| return ExtendedTimeDelta.from_timedelta(parent_result) | ||
|
|
||
| def __mul__(self, other: Union[timedelta, 'ExtendedTimeDelta']) -> Self: |
There was a problem hiding this comment.
issue: The mul method signature and implementation do not match typical timedelta behavior.
Update the type hint and docstring to indicate that mul should accept a number (int or float), not a timedelta. The implementation should raise a TypeError if 'other' is not numeric. Also, implement rmul for commutative support.
| ), | ||
| ) | ||
|
|
||
| def __dict__(self) -> dict: |
There was a problem hiding this comment.
issue: Overriding dict as a method is non-standard and may break introspection.
dict should remain an attribute; using it as a method can cause compatibility issues. Please rename the method to something like to_dict.
Comment on lines
+704
to
+718
| for index, test in enumerate(tests): | ||
| with self.subTest( | ||
| f'Testing method which_of_year (subtest {index}) with inputs: {test}', | ||
| ): | ||
| test_input_weekday, test_input_date, test_expected_output = test | ||
|
|
||
| output = TemporalAdjuster.which_of_year( | ||
| test_input_weekday, | ||
| test_input_date, | ||
| ) | ||
|
|
||
| self.assertEqual( | ||
| output, | ||
| test_expected_output, | ||
| ) |
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
| sys.path.insert(0, os.path.abspath('..')) | ||
|
|
||
| project = pyproject['project']['name'] | ||
| copyright = f'2024-{date.today().year}, {pyproject["project"]["maintainers"][0]["name"]}' |
There was a problem hiding this comment.
issue (code-quality): Don't assign to builtin variable copyright (avoid-builtin-shadow)
Explanation
Python has a number ofbuiltin variables: functions and constants thatform a part of the language, such as
list, getattr, and type(See https://docs.python.org/3/library/functions.html).
It is valid, in the language, to re-bind such variables:
list = [1, 2, 3]However, this is considered poor practice.
- It will confuse other developers.
- It will confuse syntax highlighters and linters.
- It means you can no longer use that builtin for its original purpose.
How can you solve this?
Rename the variable something more specific, such as integers.
In a pinch, my_list and similar names are colloquially-recognized
placeholders.
| (2, 30) | ||
|
|
||
| """ | ||
| if isinstance(other, int) or isinstance(other, float): |
There was a problem hiding this comment.
suggestion (code-quality): Merge isinstance calls (merge-isinstance)
Suggested change
| if isinstance(other, int) or isinstance(other, float): | |
| if isinstance(other, (int, float)): |
Comment on lines
+466
to
+467
| parent_str = super().__str__() | ||
| if parent_str: |
There was a problem hiding this comment.
suggestion (code-quality): Use named expression to simplify assignment and conditional (use-named-expression)
Suggested change
| parent_str = super().__str__() | |
| if parent_str: | |
| if parent_str := super().__str__(): |
| minute = int(seconds // 60) | ||
| seconds %= 60 | ||
|
|
||
| return time(hour, minute, int(seconds), int((seconds - int(seconds)) * 1e6)) |
There was a problem hiding this comment.
suggestion (code-quality): We've found these issues:
- Remove unnecessary casts to int, str, float or bool [×2] (
remove-unnecessary-cast) - Simplify binary operation (
bin-op-identity)
Suggested change
| return time(hour, minute, int(seconds), int((seconds - int(seconds)) * 1e6)) | |
| return time(hour, minute, seconds, int(0 * 1e6)) |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
PR Type
Enhancement, Tests, Documentation
Description
• Added comprehensive new functionality including
ExtendedTimeDeltaclass with months/years support, time operations module, and absolute date operations• Enhanced weekday operations with new
which_of_monthandwhich_of_yearmethods for determining weekday occurrences• Modernized codebase with pipe union type hints (
|) and improved formatting consistency• Added extensive test coverage for all new modules and functionality
• Implemented comprehensive documentation system with Sphinx, dark theme CSS, and automated API generation
• Enhanced CI/CD with Python 3.13 support, package quality workflows, security scanning, and pre-commit hooks
• Improved development tooling with Windows batch scripts, enhanced Makefile targets, and comprehensive linting rules
• Added project governance with code of conduct and updated dependency management
Diagram Walkthrough
File Walkthrough
9 files
weekday_operations.py
Modernize type hints and add weekday occurrence methodstemporal_adjuster/modules/weekday_operations.py
• Updated type hints from
Unionsyntax to modern pipe union syntax (|)• Standardized docstring indentation from spaces to tabs
• Added new
methods
which_of_monthandwhich_of_yearfor determining weekdayoccurrences
• Improved code formatting and line breaks for better
readability
__extended_timedelta.py
Add ExtendedTimeDelta class with months and years supporttemporal_adjuster/common/types/__extended_timedelta.py
• Added new
ExtendedTimeDeltaclass extending Python'stimedeltawithmonths and years support
• Implemented comprehensive arithmetic
operations, comparisons, and conversion methods
• Added support for
fractional time units and custom days-per-month/year configurations
•
Included serialization support and various utility methods for time
calculations
time_operations.py
Add comprehensive time operations moduletemporal_adjuster/modules/time_operations.py
• Added new
_TimeAdjusterclass with time manipulation methods•
Implemented time difference calculation, range checking, rounding, and
conversion utilities
• Added support for handling time objects that
cross midnight boundaries
absolute_date_operations.py
Add absolute date operations moduletemporal_adjuster/modules/absolute_date_operations.py
• Added new
_AbsoluteDateOperationsclass for date-integer conversions• Implemented methods for converting between dates and
day-of-year/month integers
• Added comprehensive docstrings with
examples for all methods
sequence_processor.py
Enhanced type annotations and documentation for sequence processordecoratortemporal_adjuster/common/decorators/sequence_processor.py
• Improves type annotations with
ParamSpecand proper generic types•
Adds comprehensive docstring explaining the decorator's functionality
• Refactors logic flow for better readability and maintainability
•
Adds compatibility import for
ParamSpecfromtyping_extensionstemporal_adjuster.py
Extended TemporalAdjuster class with new module integrationstemporal_adjuster/temporal_adjuster.py
• Adds imports for new module classes (
_AbsoluteDateOperations,_TimeAdjuster)• Extends class inheritance to include additional
functionality modules
• Updates class docstring formatting and removes
unnecessary
passstatementdates.py
Enhanced type definitions with time types and documentationtemporal_adjuster/common/types/dates.py
• Adds comprehensive module docstring and type documentation
•
Introduces new time-related type aliases (
AnyTime,TimeT)• Improves
existing type annotations with proper documentation comments
__init__.py
Extended module exports with new operation classestemporal_adjuster/modules/init.py
• Adds imports for new module classes (
_AbsoluteDateOperations,_TimeAdjuster)• Expands module interface to include additional
functionality
__init__.py
Extended type exports with time types and ExtendedTimeDeltatemporal_adjuster/common/types/init.py
• Adds import for new
ExtendedTimeDeltaclass• Includes new
time-related type exports (
AnyTime,TimeT)4 files
test_weekday_operations.py
Add tests for new weekday occurrence methodstests/test_weekday_operations.py
• Updated test formatting with consistent trailing commas and line
breaks
• Added comprehensive tests for new
which_of_monthandwhich_of_yearmethods• Improved test readability with better
parameter formatting
test_time_operations.py
Add comprehensive time operations test suitetests/test_time_operations.py
• Added comprehensive test suite for time operations functionality
•
Includes tests for time difference calculations, range checking,
rounding, and conversions
• Tests cover both single values and
sequence operations with edge cases
test_absolute_date_operations.py
Add comprehensive absolute date operations test suitetests/test_absolute_date_operations.py
• Added comprehensive test suite for absolute date operations
• Tests
cover date-to-integer conversions and integer-to-date conversions
•
Includes tests for both single values and sequences with edge cases
test_extended_timedelta.py
Add comprehensive ExtendedTimeDelta test suitetests/test_extended_timedelta.py
• Added comprehensive test suite for the new
ExtendedTimeDeltaclass•
Tests cover initialization, arithmetic operations, comparisons, and
conversions
• Includes tests for edge cases, serialization, and custom
configuration options
5 files
first_and_last_day_operations.py
Standardize docstring formatting and improve code styletemporal_adjuster/modules/first_and_last_day_operations.py
• Standardized docstring indentation from spaces to tabs
• Added
trailing commas to function calls for consistency
• Minor formatting
improvements for better code readability
test_first_and_last_days.py
Improve test formatting and readabilitytests/test_first_and_last_days.py
• Updated test formatting with consistent trailing commas and line
breaks
• Improved test readability with better parameter formatting
•
Standardized test method call formatting across all test cases
test_sequenceable.py
Code formatting improvements and modernization in sequenceable teststests/test_sequenceable.py
• Removes redundant comments and improves code formatting
• Replaces
set([...])syntax with modern set literal syntax{...}• Adds trailing
commas for better code consistency
test_performance.py
Code formatting improvements in performance teststests/test_performance.py
• Simplifies list comprehension formatting for better readability
•
Removes redundant comment lines
• Improves code consistency with
trailing comma addition
setup.py
Minor cleanup in setup.py file handlingsetup.py
• Removes unnecessary
'r'mode specifier when opening files forreading
• Minor code cleanup for file handling operations
15 files
conf.py
Enhanced Sphinx documentation configuration with dynamic metadataloadingdocs/conf.py
• Dynamically loads project metadata from
pyproject.tomlusingtomllib• Adds new Sphinx extensions (
viewcode,napoleon) and configuresNapoleon settings
• Changes HTML theme to
sphinx_rtd_themeand addsdark CSS styling
• Configures comprehensive autodoc settings for
better documentation generation
dark.css
Complete dark theme CSS implementation for documentationdocs/_static/css/dark.css
• Adds comprehensive dark theme CSS for documentation
• Implements
dark mode styling for all documentation elements
• Includes syntax
highlighting and UI component theming
README.md
Added Snyk package quality badge to READMEREADME.md
• Adds new Snyk package quality badge to project status indicators
index.rst
Enhanced documentation homepage with examples and better structuredocs/index.rst
• Enhances documentation homepage with project description and quick
examples
• Adds practical code examples demonstrating library usage
•
Improves table of contents structure and navigation
CODE_OF_CONDUCT.md
Added project code of conduct guidelinesCODE_OF_CONDUCT.md
• Establishes project code of conduct with emphasis on professionalism
and neutrality
• Provides guidelines for inclusive and respectful
contribution practices
temporal_adjuster.modules.rst
Auto-generated API documentation for modules packagedocs/temporal_adjuster.modules.rst
• Auto-generated Sphinx documentation file for modules package
•
Provides comprehensive API documentation structure for all module
components
ta.rst
Restructured main API documentation with comprehensive sectionsdocs/ta.rst
• Restructures main documentation page with detailed API reference
•
Adds comprehensive sections for types, enums, exceptions, and
utilities
• Improves documentation organization and navigation
CHANGELOG.md
Updated changelog with version 1.3.0 feature additionsCHANGELOG.md
• Documents new features for version 1.3.0 including
day_of_yearmethod
• Adds information about new weekday methods and numpy
vectorization improvements
temporal_adjuster.rst
Auto-generated main package API documentationdocs/temporal_adjuster.rst
• Auto-generated Sphinx documentation file for main temporal_adjuster
package
• Provides structured API documentation with subpackages and
modules
temporal_adjuster.common.enums.rst
Auto-generated API documentation for common enumsdocs/temporal_adjuster.common.enums.rst
• Auto-generated Sphinx documentation for common enums package
•
Documents day_of_week module and package structure
temporal_adjuster.common.decorators.rst
Auto-generated API documentation for common decoratorsdocs/temporal_adjuster.common.decorators.rst
• Auto-generated Sphinx documentation for common decorators package
•
Documents sequence_processor module and package structure
temporal_adjuster.common.types.rst
Auto-generated API documentation for common typesdocs/temporal_adjuster.common.types.rst
• Auto-generated Sphinx documentation for common types package
•
Documents dates module and package structure
temporal_adjuster.common.exceptions.rst
Auto-generated API documentation for common exceptionsdocs/temporal_adjuster.common.exceptions.rst
• Auto-generated Sphinx documentation for common exceptions package
•
Documents common exceptions module and package structure
temporal_adjuster.common.rst
Auto-generated API documentation for common packagedocs/temporal_adjuster.common.rst
• Auto-generated Sphinx documentation for common package
• Provides
comprehensive structure for all common subpackages
modules.rst
Auto-generated root documentation file for modulesdocs/modules.rst
• Auto-generated Sphinx documentation root file for module structure
•
Provides entry point for comprehensive API documentation
11 files
CI.yml
CI workflow updates with Python 3.13 support and streamlined testing.github/workflows/CI.yml
• Adds Python 3.13 support across all operating systems
• Updates
GitHub Actions versions and dependency file references
• Removes
coverage reporting and threshold checking from CI workflow
package_quality.yml
New comprehensive package quality assurance workflow.github/workflows/package_quality.yml
• Creates comprehensive package quality workflow with multiple checks
• Includes Ruff formatting, coverage testing, security scanning, and
leak detection
• Integrates Snyk security analysis and CodeQL scanning
.pre-commit-config.yaml
Enhanced pre-commit configuration with security and quality checks.pre-commit-config.yaml
• Updates all hook versions to latest releases
• Adds new hooks for
security (gitleaks), type checking (mypy), and code quality
• Includes
documentation building and project validation hooks
make.bat
Windows batch script for development commandsmake.bat
• Creates Windows batch script equivalent of Makefile
• Provides
commands for testing, building, documentation, and development setup
ruff.toml
Comprehensive Ruff configuration with extensive linting rulesruff.toml
• Significantly expands linting rules to include comprehensive code
quality checks
• Configures advanced formatting options and per-file
rule exceptions
• Updates target Python version and adds preview
features
readthedocs.yml
Automated Sphinx documentation building and publishing workflow.github/workflows/readthedocs.yml
• Creates automated workflow for building and publishing Sphinx
documentation
• Configures automatic documentation updates on main
branch pushes
make.bat
Added API documentation generation command to Windows build scriptdocs/make.bat
• Adds
apidoccommand for generating Sphinx API documentation•
Extends Windows documentation build capabilities
.readthedocs.yaml
Updated Read the Docs configuration with modern versions.readthedocs.yaml
• Updates Read the Docs configuration to Ubuntu 24.04 and Python 3.12
• Restructures configuration for better documentation building
Makefile
Enhanced Makefile with setup target and improved testingMakefile
• Adds new
setuptarget for development environment initialization•
Includes coverage reporting in test target and improves build process
Makefile
Added API documentation generation target to docs Makefiledocs/Makefile
• Adds
apidoctarget for generating Sphinx API documentation• Extends
documentation build capabilities with automated API doc generation
pyproject.toml
Added type information package data configurationpyproject.toml
• Adds setuptools package data configuration for type information
files
2 files
requirements.dev.txt
Reorganized development dependencies with security updatesrequirements.dev.txt
• Reorganizes development dependencies and adds numpy as direct
requirement
• Updates security-pinned package versions for
vulnerability mitigation
requirements.txt
Documentation build requirements specificationdocs/requirements.txt
• Specifies documentation build dependencies including Sphinx and
theme
• Ensures consistent documentation building environment
1 files
publish.yml
Fixed dependency file reference in publish workflow.github/workflows/publish.yml
• Updates dependency file reference from
requirements_dev.txttorequirements.dev.txt8 files