Foundation for Xcode 10 Beta 1-2 #4375

chamons · 2018-06-29T22:27:24Z

No description provided.

monojenkins · 2018-06-29T23:25:32Z

Build failure

!!! Couldn't read commit file !!!

monojenkins · 2018-06-30T01:50:35Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (please review changes)
✅ Generator Diff (please review changes)
✅ Test run succeeded

spouliot · 2018-07-02T01:21:20Z

src/foundation.cs

+		[Static]
+		[Export ("archivedDataWithRootObject:requiringSecureCoding:error:")]
+		[return: NullAllowed]
+		NSData ArchivedDataWithRootObject (NSObject @object, bool requiresSecureCoding, [NullAllowed] out NSError error);


WithRootObject is the first parameter, not the method name (you can double check the swift name)

Agreed - However I was matching the existing name:

https://github.com/xamarin/xamarin-macios/blob/master/src/foundation.cs#L2287

since it's the same method with just new params, and we're obsoleting one for another.

if we value clear naming over consistency though, I'm happy to change.

Maybe put this inside a XAMCORE_4_0 so we do not forget?

I'm not opposed, but i'm just not sure it's worth making making move from ArchivedDataWithRootObject -> GetArchivedData. I'm happy to do so however.

I think we should rename / obsolete bad APIs whenever we can. It happened to me several times already this season. Sometimes you also realize the name was a poor choice thanks to a new API. As a rule of thumb I really believe we should fix old mistakes whenever we can.

stephen-hawley · 2018-07-02T13:53:16Z

👀

dalexsoto

Otherwise it looks good to me

dalexsoto · 2018-07-02T16:19:08Z

src/foundation.cs

+	{
+		[Static]
+		[Export ("allowedTopLevelClasses", ArgumentSemantic.Copy)]
+		Class[] AllowedTopLevelClasses { get; }


Maybe a Wrap to have an array of Type

dalexsoto · 2018-07-02T16:21:06Z

src/foundation.cs

+
+		[Watch (5,0), NoTV, NoMac, iOS (12,0)]
+		[Static]
+		[Export ("deleteSavedUserActivitiesWithPersistentIdentifiers:completionHandler:")]


Maybe we can add [Async] here

dalexsoto · 2018-07-02T16:21:20Z

src/foundation.cs

+
+		[Watch (5,0), NoTV, NoMac, iOS (12,0)]
+		[Static]
+		[Export ("deleteAllSavedUserActivitiesWithCompletionHandler:")]


Same, [Async]

chamons · 2018-07-02T17:00:19Z

I added the code review changes and a test for the new [Wrap].

I'm currently only on laptop and can't test it locally.

monojenkins · 2018-07-02T19:26:09Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (please review changes)
✅ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

9 tests failed, 55 tests passed.

Failed tests

monotouch-test/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
monotouch-test/tvOS - simulator/Debug: Failed
monotouch-test/tvOS - simulator/Debug (static registrar): Failed
monotouch-test/tvOS - simulator/Release (all optimizations): Failed
monotouch-test/watchOS - simulator/Debug: Failed
monotouch-test/watchOS - simulator/Debug (static registrar): Failed
monotouch-test/watchOS - simulator/Release (all optimizations): Failed

spouliot · 2018-07-03T14:03:57Z

tests/monotouch-test/Foundation/NSKeyedUnarchiverTest.cs

+
+			NSDictionary<NSString, NSString> testValues = new NSDictionary<NSString, NSString> ((NSString)"1", (NSString)"a");
+			NSData data = NSKeyedArchiver.ArchivedDataWithRootObject (testValues, true, out NSError error);
+			Assert.IsNull (error);


name the asserts, too many reports (and sometimes bad symbols) makes debugging failures harder than it have to be

spouliot · 2018-07-03T14:05:24Z

tests/monotouch-test/Foundation/NSKeyedUnarchiverTest.cs

+		public void GetUnarchivedObject_TypeWrappers ()
+		{
+			if (!TestRuntime.CheckXcodeVersion (10, 0))
+				Assert.Ignore ("Ignoring GetUnarchivedObject_TypeWrappers as requires new APIs");


minor: you can assert the Xcode version (shorter than a version check + assert)

spouliot · 2018-07-03T14:05:29Z

tests/monotouch-test/Foundation/NSKeyedUnarchiverTest.cs

+		public void DataTransformer_AllowedTopLevelTypes_WrapperTests ()
+		{
+			if (!TestRuntime.CheckXcodeVersion (10, 0))
+				Assert.Ignore ("Ignoring DataTransformer_AllowedTopLevelTypes_WrapperTests as requires new APIs");


minor: you can assert the Xcode version (shorter than a version check + assert)

VincentDondain · 2018-07-03T15:17:48Z

src/foundation.cs

+	[Watch (5,0), TV (12,0), Mac (10,14, onlyOn64: true), iOS (12,0)]
+	[BaseType (typeof(NSValueTransformer))]
+	interface NSSecureUnarchiveFromDataTransformer
+	{


Minor: styling, this goes 1 line higher (:

monojenkins · 2018-07-11T00:32:54Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (please review changes)
✅ Generator Diff (please review changes)
🔥 Test run failed 🔥

Test results

9 tests failed, 55 tests passed.

Failed tests

monotouch-test/iOS Unified 64-bits - simulator/Debug: Failed
monotouch-test/iOS Unified 64-bits - simulator/Debug (static registrar): Failed
monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed
monotouch-test/tvOS - simulator/Debug: Failed
monotouch-test/tvOS - simulator/Debug (static registrar): Failed
monotouch-test/tvOS - simulator/Release (all optimizations): Failed
monotouch-test/watchOS - simulator/Debug: Failed
monotouch-test/watchOS - simulator/Debug (static registrar): Failed
monotouch-test/watchOS - simulator/Release (all optimizations): Failed

chamons · 2018-07-11T21:38:03Z

Looks like a legit ios failure - DataTransformer_AllowedTopLevelTypes_WrapperTests

chamons · 2018-07-13T15:49:15Z

I'm punting on those bindings until I fix - #4441

This PR has waited long enough already.

monojenkins · 2018-07-13T18:16:25Z

Build success
✅ Build succeeded
✅ API Diff (from stable)
✅ API Diff (from PR only) (please review changes)
✅ Generator Diff (please review changes)
✅ Test run succeeded

dalexsoto

LGTM 👍

Things pointed out are fixed, also dismissing because FTO

chamons added 8 commits June 29, 2018 11:29

NSKeyedArchiver/NSKeyedUnarchiver

41db00f

More bindings

1807e46

Rest of bindings

e21d401

Fix xtro

b7c318b

Fix messages

3a77360

Fix more messages

0450279

Remove empty todo

9b7768e

Add basic test for NSKeyUnarchiever wrappers

d74694c

chamons requested a review from spouliot June 29, 2018 22:27

chamons requested a review from rolfbjarne as a code owner June 29, 2018 22:27

Fix two ugly method names

86ce462

spouliot added this to the xcode10 milestone Jul 2, 2018

spouliot reviewed Jul 2, 2018

View reviewed changes

dalexsoto approved these changes Jul 2, 2018

View reviewed changes

dalexsoto reviewed Jul 2, 2018

View reviewed changes

Code review and test for wrapping

193a859

rolfbjarne approved these changes Jul 2, 2018

View reviewed changes

dalexsoto approved these changes Jul 2, 2018

View reviewed changes

spouliot previously requested changes Jul 3, 2018

View reviewed changes

VincentDondain reviewed Jul 3, 2018

View reviewed changes

VincentDondain approved these changes Jul 3, 2018

View reviewed changes

Code review changes

0c7d670

chamons mentioned this pull request Jul 13, 2018

[generator] Binding with return value of Class [] do not do the right thing #4441

Closed

Work around dotnet#4441 by commenting out binding until fixed

9680cad

dalexsoto approved these changes Jul 13, 2018

View reviewed changes

chamons merged commit 6d039cb into dotnet:xcode10 Jul 13, 2018

chamons deleted the xcode10-foundation branch July 13, 2018 18:27

Foundation for Xcode 10 Beta 1-2 #4375

Foundation for Xcode 10 Beta 1-2 #4375

Uh oh!

Conversation

chamons commented Jun 29, 2018

Uh oh!

monojenkins commented Jun 29, 2018

Uh oh!

monojenkins commented Jun 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephen-hawley commented Jul 2, 2018

Uh oh!

dalexsoto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chamons commented Jul 2, 2018

Uh oh!

monojenkins commented Jul 2, 2018

Test results

Failed tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monojenkins commented Jul 11, 2018

Test results

Failed tests

Uh oh!

chamons commented Jul 11, 2018

Uh oh!

chamons commented Jul 13, 2018

Uh oh!

monojenkins commented Jul 13, 2018

Uh oh!

dalexsoto left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants