expr: Convert date time accessors to properties #571
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to c0fd502 in 1 minute and 1 seconds
More details
- Looked at
556lines of code in25files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. fennel/CHANGELOG.md:3
- Draft comment:
The changelog entry mentions only some of the datetime accessors that were changed to properties. Consider listing all the accessors that were changed for clarity. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes datetime accessors to properties, but the changelog entry is missing some of the accessors that were changed.
Workflow ID: wflow_ecKsQvy4JhoNWxyG
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
fennel/featuresets/featureset.py
Outdated
| @@ -103,6 +103,7 @@ def feature( | |||
| if isinstance(args[0], Expr): | |||
| feature_obj._expr = args[0] | |||
| else: | |||
| print("Setting ref", args[0]) | |||
There was a problem hiding this comment.
Remove the print statement as it seems to be a leftover debug statement.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aditya-nambiar
force-pushed
the
aditya/make-hour-property
branch
from
c0fd502
to
7c27a0f
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7c27a0f in 37 seconds
More details
- Looked at
534lines of code in24files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. fennel/CHANGELOG.md:5
- Draft comment:
The changelog entry for version 1.5.30 is missing some datetime accessors that were converted to properties. Ensure all accessors (year,month,day,hour,minute,second,millisecond,microsecond,week) are listed. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_amYvmiiEHWpMFnut
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
aditya-nambiar
force-pushed
the
aditya/make-hour-property
branch
from
7c27a0f
to
fa5d9fc
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on fa5d9fc in 59 seconds
More details
- Looked at
553lines of code in24files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_sdInLEeRognVxJQ1
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| StringNoop(), | ||
| ) | ||
|
|
||
| def year(self, timezone: Optional[str] = "UTC") -> _Number: | ||
| return self.parts(TimeUnit.YEAR, timezone) | ||
| def with_tz(self, timezone: str) -> _DateTime: |
There was a problem hiding this comment.
The with_tz method modifies the instance's timezone, which can lead to unexpected behavior if the same instance is reused. Consider returning a new instance with the updated timezone instead.
aditya-nambiar
force-pushed
the
aditya/make-hour-property
branch
from
fa5d9fc
to
d85f253
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on d85f253 in 39 seconds
More details
- Looked at
562lines of code in24files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. fennel/CHANGELOG.md:5
- Draft comment:
The changelog entry for version 1.5.30 is missing some datetime accessors that were converted to properties. Ensure all accessors likeyear,month,day,hour,minute,second,millisecond,microsecond, andweekare mentioned. - Reason this comment was not posted:
Confidence changes required:50%
The PR changes datetime accessors to properties, but the changelog entry is missing some of the accessors that were changed. It should include all the accessors that were converted to properties.
Workflow ID: wflow_xuAzW6V4pP7R3YlG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Important
Convert datetime accessors to properties in
fennel/expr/expr.pyand update related tests, documentation, and versioning.year,month,day,hour,minute,second,millisecond,microsecond,week) to properties infennel/expr/expr.py.year_with_tz,month_with_tz, etc.) for datetime accessors.test_expr.pyandtest_featureset.pyto use new datetime properties.dt.pyto reflect changes in datetime accessors.CHANGELOG.mdto document changes in version 1.5.30.pyproject.toml.featureset.py.This description was created by
for d85f253. It will automatically update as commits are pushed.