[CoreData] Updated bindings for xcode9. #2210

mandel-macaque · 2017-06-11T16:34:15Z

No description provided.

monojenkins · 2017-06-11T17:45:50Z

Build failure

spouliot · 2017-06-12T01:01:06Z

src/CoreData/Enums.cs

@@ -51,6 +51,10 @@ public enum NSAttributeType : nuint {
 		Boolean = 800,
 		Date = 900,
 		Binary = 1000,
+		[iOS (11,0), TV (11,0), Mac (10,3), Watch (4,0)]


Un likely to be mac 10,3

spouliot · 2017-06-12T01:01:19Z

src/CoreData/Enums.cs

@@ -51,6 +51,10 @@ public enum NSAttributeType : nuint {
 		Boolean = 800,
 		Date = 900,
 		Binary = 1000,
+		[iOS (11,0), TV (11,0), Mac (10,3), Watch (4,0)]
+		Uuid = 1100,
+		[iOS (11,0), TV (11,0), Mac (10,3), Watch (4,0)]


spouliot · 2017-06-12T01:02:15Z

src/CoreData/Enums.cs

-		StringPatternMatching = 1680
+		StringPatternMatching = 1680,
+		[iOS (11,0), TV (11,0), Mac (10,13), Watch (4,0)]
+		InvalidUri = 1690


Add a comma
It makes future diff cleaner and the compiler allows it

spouliot · 2017-06-12T01:02:27Z

src/CoreData/Enums.cs

-		ExternalRecordImport = 134200
+		ExternalRecordImport = 134200,
+		[iOS (11,0), TV (11,0), Mac (10,3), Watch (4,0)]
+		HistoryTokenExpired = 134301


spouliot · 2017-06-12T01:06:20Z

src/coredata.cs

+	interface NSPersistentHistoryChange : NSCopying
+	{
+		[Export ("changeID")]
+		long ChangeID { get; }


ID inconsistant with next api
Iirc we use Id (abbrev not acronym)

spouliot · 2017-06-12T01:09:19Z

src/coredata.cs

+		[NoWatch][NoTV]
+		[Since (7,0), Mavericks]
+		[Field ("NSPersistentStoreUbiquitousContainerIdentifierKey")]
+		NSString UbiquitousContainerIdentifierKey { get; }


Good catch, we should add this test in intro

Do you want me to add an intro test that makes sure that the Fields are correctly expelled? Or to add it for this one in particular?

In a separate pr you can add a check that public fields (all members in fact) should start with an uppercase letter unless it has an obsolete attribute

spouliot · 2017-06-12T01:10:39Z

src/coredata.cs

+
+		[Watch (4, 0), TV (11, 0), Mac (10, 13), iOS (11, 0)]
+		[Field ("NSPersistentHistoryTrackingKey")]
+		NSString TrackingKey { get; }


Are not we losing History from the full name?

dalexsoto · 2017-06-12T04:22:44Z

src/coredata.cs

+	{
+		[Static]
+		[Export ("fetchHistoryAfterDate:")]
+		NSPersistentHistoryChangeRequest FetchHistoryAfterDate (NSDate date);


I think you can do FetchHistoryAfter so this and the next 3 can be overloads

Perfectly valid idea. Doing so.

dalexsoto · 2017-06-12T04:23:55Z

src/coredata.cs

+
+		[Static]
+		[Export ("deleteHistoryBeforeDate:")]
+		NSPersistentHistoryChangeRequest DeleteHistoryBeforeDate (NSDate date);


Same here, I think these 3 can ve overloads DeleteHistoryBefore

Ditto as with the previous one.

dalexsoto · 2017-06-12T04:24:48Z

src/coredata.cs

+		long TransactionNumber { get; }
+
+		[Export ("storeID")]
+		string StoreId { get; }


I think we want to keep here as ID instead of Id to keep consistency

@spouliot so I forgot an ID to Id, yet @dalexsoto has this comment. I'm +1 on Id (did it everywhere but one that I forgot). What do you guys prefer?

Yeah @spouliot is right we use ID not as an acronym so it should be Id totally my bad

VincentDondain · 2017-06-12T16:03:46Z

src/coredata.cs

@@ -263,12 +309,22 @@ interface NSEntityDescription : NSCoding, NSCopying {
 		[Since(5,0)]
 		[NullAllowed] // by default this property is null
 		[Export ("compoundIndexes", ArgumentSemantic.Retain)]
+		[Availability (Introduced = Platform.iOS_5_0 | Platform.Mac_10_7, Deprecated = Platform.iOS_11_0 | Platform.Mac_10_13, Message = "Use NSEntityDescription.Indexes instead.")]


Please use apostrophes in your message (:

Use 'NSEntityDescription.Indexes' instead.

https://github.com/xamarin/xamarin-macios/wiki/BINDINGS#rule-2

VincentDondain · 2017-06-12T16:05:14Z

src/coredata.cs

@@ -1947,6 +2172,7 @@ interface NSPropertyDescription : NSCoding, NSCopying {
 		NSDictionary UserInfo { get; set; }

 		[Export ("indexed")]
+		[Availability (Introduced = Platform.iOS_3_0 | Platform.Mac_10_5 , Deprecated = Platform.iOS_11_0 | Platform.Mac_10_13, Message = "Use NSEntityDescription.Indexes instead")]


Same as https://github.com/xamarin/xamarin-macios/pull/2210/files#r121455209

VincentDondain · 2017-06-12T16:11:43Z

src/coredata.cs

@@ -1966,6 +2192,7 @@ interface NSPropertyDescription : NSCoding, NSCopying {

 		[Since (5,0)]
 		[Export ("storedInExternalRecord")]
+		[Availability (Introduced = Platform.iOS_3_0 | Platform.Mac_10_5 , Deprecated = Platform.iOS_11_0 | Platform.Mac_10_13, Message = "Spotlight integration is deprecated. Use CoreSpotlight integration instead.")]


Please put CoreSpotlight between apostrophes. 'CoreSpotlight'

monojenkins · 2017-06-12T16:28:40Z

Build failure

spouliot · 2017-06-12T17:41:44Z

@mandel pretty sure Id is what the FXDG promotes

monojenkins · 2017-06-13T19:28:53Z

Build failure

is supported. A radar has been created on Apples side: https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

mandel-macaque · 2017-06-14T09:20:49Z

The new revision has the test changed to pass a radar has been filed regarding the issues found by the introspection tests:

https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

mandel-macaque · 2017-06-14T10:16:24Z

src/coredata.cs

+
+	[Watch (4,0), TV (11,0), Mac (10,13), iOS (11,0)]
+	[BaseType (typeof (NSObject))]
+	interface NSPersistentHistoryToken : NSCopying //, NSSecureCoding TODO: The class does state that it supports the NSSecureCoding YET SupportsSecureCoding returns false, radar 32761925


@spouliot @rolfbjarne is this the correct way? I updated the tests, maybe is not needed to comment it out. Your call :)

Fine, make sure to tag the Trello card (for the radar) that an action is needed once apple fix it (with a link to the pr / comment)

Added it already, just added the line in the comments: https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

monojenkins · 2017-06-14T12:25:22Z

Build failure

monojenkins · 2017-06-14T16:26:10Z

Build failure

monojenkins · 2017-06-15T12:41:05Z

Build success

spouliot · 2017-06-15T15:28:32Z

src/coredata.cs

+
+	[Watch (4,0), TV (11,0), Mac (10,13), iOS (11,0)]
+	[BaseType (typeof(NSPersistentStoreRequest))]
+	interface NSPersistentHistoryChangeRequest


I wonder if the default ctor is usable ? Considering all the static methods to create instances

Unless it make sense (base types) I think we should disable the default .ctor
Easier to add later than to remove a broken API

spouliot · 2017-06-15T15:31:04Z

tests/monotouch-test/monotouch-test.csproj

@@ -752,7 +756,6 @@
  </ItemGroup>
  <ItemGroup>
    <EmbeddedResource Include="Security\openssl_crt.der">
-        <LogicalName>monotouchtest.Security.openssl_crt.der</LogicalName>


That looks like a mistake

Ha, That is visual studio doing something in my back.. cheeky..

spouliot · 2017-06-15T15:32:40Z

tests/monotouch-test/CoreData/NSQueryGenerationTokenTest.cs

+		[Test]
+		public void EncodeWithCoderTest ()
+		{
+			if (!PlatformInfo.Host.IsArch32 ) {


Simpler to check if intptr == 8

Maybe our check for Xcode 9 should only return true on 64 bits (on iOS only) ?

@spouliot PlatformInfo.Host.IsArch32 does the IntPtr.Size check so why not use it? just curious 😄

Not critical but it avoids adding another file + consistant with other tests

@mandel that test also need a version check for Xcode 9 or it will fail on older versions

rolfbjarne · 2017-06-15T16:00:01Z

src/coredata.cs

@@ -1818,8 +2029,24 @@ partial interface NSPersistentStoreCoordinator
 		[NoWatch][NoTV]
 		[Since (7,0), Mavericks]
 		[Field ("NSPersistentStoreUbiquitousContainerIdentifierKey")]
+		[Obsolete ("Use UbiquitousContainerIdentifierKey instead.")]


I think this should be "Use 'UbiquitousContainerIdentifierKey' instead."

monojenkins · 2017-06-16T14:57:27Z

Build failure

monojenkins · 2017-06-16T18:52:02Z

Build success

dalexsoto

Minor thingy

dalexsoto · 2017-06-17T01:53:35Z

src/CoreData/Enums.cs

+		TransactionsOnly = 3,
+		ChangesOnly = 4,
+		TransactionsAndChanges = 5
+}


indentation

rolfbjarne · 2017-06-19T08:00:05Z

tests/monotouch-test/CoreData/NSQueryGenerationTokenTest.cs

+		public void EncodeWithCoderTest ()
+		{
+			// Added test to ensure we do support NSCoding even when introspection fails.
+			if (IntPtr == 8  && if (TestRuntime.CheckExactXcodeVersion (11, 0, beta: 1)) {


You have an if inside the if condition, that shouldn't compile...

Yet it did.. WEIRD!

rolfbjarne · 2017-06-19T08:00:27Z

tests/monotouch-test/monotouch-test.csproj

@@ -1,4 +1,4 @@
-<?xml version="1.0" encoding="utf-8"?>
+<?xml version="1.0" encoding="utf-8"?>


This file only has whitespace changes, you don't need it in the diff at all.

is supported. A radar has been created on Apples side: https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

…acios into coredata-xcode9

monojenkins · 2017-06-19T10:50:04Z

Build failure

monojenkins · 2017-06-19T10:50:05Z

Build failure

monojenkins · 2017-06-19T11:00:19Z

Build failure

monojenkins · 2017-06-19T11:10:14Z

Build failure

rolfbjarne · 2017-06-19T12:17:30Z

tests/monotouch-test/CoreData/NSQueryGenerationTokenTest.cs

+		public void EncodeWithCoderTest ()
+		{
+			// Added test to ensure we do support NSCoding even when introspection fails.
+			if (IntPtr.Size == 8  && TestRuntime.CheckExactXcodeVersion (9, 0, beta: 1)) {


Why did you add an exact check for Xcode 9 b1?

In your case it means this test won't run on any other Xcode version (usually exact Xcode checks like this are used to skip a test for a specific Xcode version, so that the test runs in later Xcode versions, which allows us to manually verify if an SDK bug was fixed (or not)).

Also: is this API available on watchOS? In which case this API would still be 32-bit.

I think you'll just want to do this:

if (TestRuntime.CheckXcodeVersion (9, 0))

monojenkins · 2017-06-19T12:23:21Z

Build failure

monojenkins · 2017-06-19T15:55:26Z

Build failure

monojenkins · 2017-06-19T17:07:59Z

Build success

Updated bindings for xcode9.

082ecc1

dnfclas added the cla-already-signed label Jun 11, 2017

spouliot suggested changes Jun 12, 2017

View reviewed changes

dalexsoto requested changes Jun 12, 2017

View reviewed changes

Updated according to reviews.

430c626

VincentDondain reviewed Jun 12, 2017

View reviewed changes

rolfbjarne approved these changes Jun 13, 2017

View reviewed changes

Fix some of the test failures.

79284b0

Fixed the tests to pass in those cases that NSCoding and NSSecureCoding

f2b3108

is supported. A radar has been created on Apples side: https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

mandel-macaque commented Jun 14, 2017

View reviewed changes

Do fix tests on the mac due to a typo.

fa09f1d

Fix test on 32.

c0333e7

dalexsoto approved these changes Jun 15, 2017

View reviewed changes

spouliot suggested changes Jun 15, 2017

View reviewed changes

rolfbjarne requested changes Jun 15, 2017

View reviewed changes

Minos changes as per reviews.

4b36e21

spouliot approved these changes Jun 16, 2017

View reviewed changes

dalexsoto approved these changes Jun 17, 2017

View reviewed changes

src/CoreData/Enums.cs Outdated

TransactionsOnly = 3,

ChangesOnly = 4,

TransactionsAndChanges = 5

}

Copy link

Member

dalexsoto Jun 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

rolfbjarne requested changes Jun 19, 2017

View reviewed changes

mandel-macaque added 10 commits June 19, 2017 12:14

Updated bindings for xcode9.

1f43c0f

Updated according to reviews.

f12c917

Fix some of the test failures.

8be640d

Fixed the tests to pass in those cases that NSCoding and NSSecureCoding

15c3ee7

is supported. A radar has been created on Apples side: https://trello.com/c/y87bjnr9/69-32761925-nspersistenthistorytoken-supports-nssecurecoding-yet-supportssecurecoding-returns-false

Do fix tests on the mac due to a typo.

3d58b2a

Fix test on 32.

2461747

Minos changes as per reviews.

eabdc7a

Fix test as per review.

a2ae0d6

Remove spaces changes.

2ab7240

Merge branch 'coredata-xcode9' of github.com:mandel-macaque/xamarin-m…

3c92aa1

…acios into coredata-xcode9

mandel-macaque force-pushed the coredata-xcode9 branch from 131395d to e73bfa0 Compare June 19, 2017 10:47

mandel-macaque force-pushed the coredata-xcode9 branch from e73bfa0 to 1512ab1 Compare June 19, 2017 10:58

Fix white spaces changes.

cdaebe1

mandel-macaque force-pushed the coredata-xcode9 branch from 1512ab1 to cdaebe1 Compare June 19, 2017 11:08

rolfbjarne reviewed Jun 19, 2017

View reviewed changes

Fix test to work on 32-bit.

39a6c93

mandel-macaque force-pushed the coredata-xcode9 branch from 0e15e4b to 39a6c93 Compare June 19, 2017 15:52

rolfbjarne approved these changes Jun 21, 2017

View reviewed changes

mandel-macaque merged commit 4eafdb1 into xamarin:xcode9 Jun 22, 2017

mandel-macaque deleted the coredata-xcode9 branch June 22, 2017 09:48

		@@ -1,4 +1,4 @@
		<?xml version="1.0" encoding="utf-8"?>
		<?xml version="1.0" encoding="utf-8"?>

[CoreData] Updated bindings for xcode9. #2210

[CoreData] Updated bindings for xcode9. #2210

Conversation

mandel-macaque commented Jun 11, 2017

monojenkins commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentDondain Jun 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 12, 2017

spouliot commented Jun 12, 2017

monojenkins commented Jun 13, 2017

mandel-macaque commented Jun 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 14, 2017

monojenkins commented Jun 14, 2017

monojenkins commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 16, 2017

monojenkins commented Jun 16, 2017

dalexsoto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jun 19, 2017

monojenkins commented Jun 19, 2017

monojenkins commented Jun 19, 2017

monojenkins commented Jun 19, 2017

rolfbjarne Jun 19, 2017 • edited Loading

Choose a reason for hiding this comment

monojenkins commented Jun 19, 2017

monojenkins commented Jun 19, 2017

monojenkins commented Jun 19, 2017

VincentDondain Jun 12, 2017 •

edited

Loading

rolfbjarne Jun 19, 2017 •

edited

Loading