-
Notifications
You must be signed in to change notification settings - Fork 428
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
Deprecate DateTime methods from issue #18846. #19955
Conversation
Deprecate and rename DateTime methods from issue chapel-lang#18846. This has not yet modified the classes or enums suggested in that issue. Added a deprecated test to trigger the warnings for each of the deprecated methods. Signed-off-by: David Iten <[email protected]>
I missed these two in the last commit. Signed-off-by: David Iten <[email protected]>
Fix tests that were using the old all lowercase names. Signed-off-by: David Iten <[email protected]>
Fix tests that were using the old all lowercase names. Signed-off-by: David Iten <[email protected]>
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.
This is fairly straightforward and is looking good to me.
If we can reduce the code duplication between deprecated methods and their newly-named replacements, that could save a bit of time later and help avoid forgetting to change one and not the other.
if tzinfo.borrow() == nil { | ||
return new timedelta(); | ||
} else { | ||
return tzinfo!.utcOffset(datetime.now()); |
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.
Can this be called by the deprecated method?
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 are some other similar cases:
- fromTimestamp
- utcFromTimestamp
- fromOrdinal
- utcOffset
- fromUtc
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.
Thanks @benharsh! I replaced the bodies of the deprecated methods with calls to their replacements.
In order to reduce code duplication just call the new versions of deprecated methods. Signed-off-by: David Iten <[email protected]>
Removes a collection of previously deprecated symbols from the Time module. The removed symbols were deprecated in the following PRs: - #21826 - #22028 - #19955 - #21467 - #21034 - a subset of the symbols in #23114 Testing - [x] paratest without comm - [x] paratest with comm - [x] built and checked docs [Reviewd by @riftEmber]
Deprecate and rename DateTime methods from issue #18846. This has not yet
modified the classes or enums suggested in that issue.
Added a deprecated test to trigger the warnings for each of the deprecated
methods.
Fixed up any tests that were using the now-deprecated names.
Signed-off-by: David Iten [email protected]