Skip to content

SR-14933: Implement copy() in ISO8601DateFormatter. #3009

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

Merged
merged 2 commits into from
Aug 21, 2021

Conversation

YOCKOW
Copy link
Member

@YOCKOW YOCKOW commented Jul 16, 2021

As with #3008, ISO8601DateFormatter should also implement its own override of copy().

Resolves SR-14933.

@spevans
Copy link
Contributor

spevans commented Jul 16, 2021

@swift-ci test

@spevans
Copy link
Contributor

spevans commented Jul 17, 2021

So interestingly I ran the test_copy on BigSur 11.4.4 using DarwinCompatibilityTests and it fails, which leads me to believe this is actually a bug in Darwin as well.

$ swift
Welcome to Apple Swift version 5.5 (swiftlang-1300.0.20.104 clang-1300.0.21.1).
Type :help for assistance.
  1> import Foundation 
  2.  
  3. let f1 = ISO8601DateFormatter() 
  4. let f2 = f1.copy() as! ISO8601DateFormatter 
  5. print(f1) 
  6. print(f2) 
  7. print(f1 === f2)
<NSISO8601DateFormatter: 0x10040dbd0>
<NSISO8601DateFormatter: 0x10040dbd0>
true

@millenomi Is ISO8601DateFormatter implementing NSCopying correctly on Darwin?

@YOCKOW I think this PR is correct I just want to see Lily's response first. I thinks its also worth adding a XCTAssertFalse(copied === original) into your tests for these copy() fixing PRs.

@YOCKOW
Copy link
Member Author

YOCKOW commented Jul 18, 2021

@spevans
I'm sorry, but I didn't know ISO8601DateFormatter on Darwin has such behavior.
I'm going to modify the test.

@spevans
Copy link
Contributor

spevans commented Jul 18, 2021

@YOCKOW I don't think your PR is wrong, it might be that there is a bug in Darwin's Foundation. I just wanted confirmation that the bug exists on macOS as well. So I would hold off on making any changes for now.

@YOCKOW
Copy link
Member Author

YOCKOW commented Jul 18, 2021

@spevans
Noted with thanks. I've just modified the test in this PR on local, but I won't push it as of now.

@YOCKOW
Copy link
Member Author

YOCKOW commented Aug 13, 2021

Date.ISO8601FormatStyle is coming, but I believe this PR is not stale yet.

@YOCKOW YOCKOW requested a review from millenomi August 13, 2021 08:52
@spevans
Copy link
Contributor

spevans commented Aug 20, 2021

@millenomi Could you check my earlier comment in #3009 (comment) it looks like Darwin's Foundation doesn't properly implement ISO8601DateFormatter.copy. I just want to confirm that Darwin has a bug

@millenomi
Copy link
Contributor

millenomi commented Aug 21, 2021

It is indeed a bug. Do you happen to have a FB number handy for it?

@spevans
Copy link
Contributor

spevans commented Aug 21, 2021

The sync system seemed to link https://bugs.swift.org/browse/SR-14108 to rdar://problem/73742491, not sure if that is the best bug to cover it, otherwise could link to https://bugs.swift.org/browse/SR-14933 which covers this fix.

@spevans spevans merged commit 1bc603e into swiftlang:main Aug 21, 2021
@YOCKOW
Copy link
Member Author

YOCKOW commented Aug 22, 2021

Should I file a separate report about that using an other bug tracker such as Feedback Assistant?

@YOCKOW YOCKOW deleted the SR-14933 branch August 22, 2021 09:23
@spevans
Copy link
Contributor

spevans commented Aug 25, 2021

@YOCKOW When you get a chance could you possibly backport this fix to the 5.5 branch (and also 5.4 if it can be done before Friday), thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants