General cleanup and Objective-C modernization #211

danieltiger · 2017-05-10T19:10:13Z

Cleanup up code inconsistencies where I saw a general preference
Switched from [[alloc] init] to [new] where appropriate
Switched from explicitly checking against nil/null to just checking for presence
Removed unneeded assignment to nil, as it's the default behavior in Objective-C
Switched to using NS_UNAVAILABLE annotation to indicate no init method, rather than implementing it and raising an assertion. This way user code won't compile with init and they'll be told why.
Flipped some check logic so that we return early in the negative case, thus collapsing branches and de-indenting code.
Switched a check to search for object presence first, before calling an external method. This way if we don't have the object we don't waste time making the call.
Switched from [[alloc] init] for dictionaries and arrays to [dictionary] and [array]
Removed validate methods that only checked for nil/length of 0. They were being used in ways that made it clear they were misunderstood. Instead just checked for things directly.
Switched KIOQuery to use [super init] instead of [self init]. This is so that if anyone ever subclasses KIOQuery they will get the expected behavior. If there is some reason it was [self init] please let me know.
Removed makeDictionaryMutable and makeArrayMutable methods. Instead just use the built-in mutableCopy.
Removed strong from property declarations as it's the default.
Changed some error messages to be more explicit about what the problem is.
Switched from rangeOfString to hasPrefix and containsString.
Check for existence of self in KeenProperties before setting property values.

…jectID:andWriteKey:andReadKey:, after they have been initially set.

…hecking for nil and empty string, but were being used in a way that indicated that wasn't clear.

… iOS 8 checking code.

… the default. Use containsString and hasPrefix instead of range checking.

…ded check for self in init.

…d confusing validate method and just check for nil/empty. No need to init to nil in Objective-C for objects.

…iver is a no-op.

…d. Other assorted minor cleanup.

coveralls · 2017-05-10T19:29:59Z

Coverage decreased (-0.06%) to 65.908% when pulling ccf6c8c on danieltiger:assorted-improvements into b3aad6c on keenlabs:master.

baumatron

Looks good! Only concern is with breaking changes that would require a major revision bump. We could throw those in a vnext_major branch until we're ready to roll a major release.

baumatron · 2017-05-11T20:16:19Z

KeenClient/KIONetwork.h

@@ -15,24 +15,25 @@
 + (instancetype)sharedInstance;

 // Initialize the object
- (instancetype)initWithURLSession:(NSURLSession*)urlSession
-                          andStore:(KIODBStore*)store;
+- (instancetype)init NS_UNAVAILABLE;


Yeah, it's a handy one!

baumatron · 2017-05-11T20:17:31Z

KeenClient/KIOQuery.m

        return nil;
    }

-    self = [self init];
+    self = [super init];


Good catch.

baumatron · 2017-05-11T20:20:29Z

KeenClient/KIOUtil.m

@@ -23,21 +23,13 @@ + (NSData *)serializeEventToJSON:(NSMutableDictionary *)event error:(NSError**)
    return [NSJSONSerialization dataWithJSONObject:fixed options:0 error:error];
 }

-+ (NSMutableDictionary *)makeDictionaryMutable:(NSDictionary *)dict {


Good to see these gone.

baumatron · 2017-05-11T20:25:00Z

KeenClient/KeenClient.m

        return nil;
    }

+    // If the keys are already set, return the existing instance, don't overwrite the keys
+    if (self.sharedClient.projectID || self.sharedClient.writeKey || self.sharedClient.readKey) {


This would technically require revving a major revision, maybe this part of the change could be pared off and thrown into a vnext_major branch until we're ready to release a major revision then bring it to master?

Yep, I'll pull it out for now.

How do you want me to do that? I can't create branches on this repo.

Just pushed a vnext_major branch we can use.

baumatron · 2017-05-11T20:28:29Z

KeenClient/KeenClient.m

-            if ([KeenClient isLocationAuthorized:clAuthStatus]) {
-                [self startMonitoringLocation];
-            }
-#if __IPHONE_OS_VERSION_MAX_ALLOWED >= 80000


I'm all for going iOS 8+. We could make a pass and update targets at some point to actually require this deployment target. The podspec is at iOS 6 right now and there's a spattering of different versions throughout the project. Technically this is a breaking change, so it would be a major revision and should probably go in a vnext_major branch.

Sounds good.

Created #214

baumatron · 2017-05-11T20:31:08Z

KeenClient/KeenLogger.m

@@ -54,13 +54,13 @@ - (KeenLogger*)init {
        // about concurrent access when adding and removing loggers
        // since those operations will be dispatched to this queue as well.
        _loggerQueue = dispatch_queue_create("io.keen.logger", DISPATCH_QUEUE_SERIAL);
-        if (NULL == _loggerQueue) {
+        if (_loggerQueue == NULL) {


Should these also be updated to if (!_loggerQueue), etc?

I wouldn't... but that's mainly a stylistic thing with me, I find if (!_loggerQueue) harder to read. If you want me to do that though, I totally can.

In general I tried to keep the existing style as much as possible, and simply clean up inconsistencies.

Nope, I'm good leaving it like this. I'm not highly opinionated about it.

danieltiger · 2017-05-11T21:01:34Z

Ok, major changes removed. I'm creating new branches and PRs for each of those.

baumatron · 2017-05-11T21:22:42Z

Looks like a couple of tests failed, but they look like they could be due to some jankiness with the tests. Changes in #212 will help make those more reliable. I'll re-run the build.

danieltiger · 2017-05-11T21:23:43Z

They are passing locally, for what that's worth :)

coveralls · 2017-05-11T21:31:21Z

Coverage decreased (-0.09%) to 65.882% when pulling 3e9dd5a on danieltiger:assorted-improvements into b3aad6c on keenlabs:master.

baumatron · 2017-05-11T21:31:55Z

Passed on this run, good to go!

danieltiger · 2017-05-11T21:35:35Z

Cool! Merge away then, or whenever you feel comfortable :)

danieltiger added 20 commits May 8, 2017 12:01

Don't allow the changing of KeenClient keys using sharedClientWithPro…

87a6f88

…jectID:andWriteKey:andReadKey:, after they have been initially set.

Removed the misleading validateProjectID and validateKey. They were c…

b883644

…hecking for nil and empty string, but were being used in a way that indicated that wasn't clear.

Reverse the initial check to collapse a large branch. Remove outdated…

804e262

… iOS 8 checking code.

Cleaned up stray *. Removed strong from property declarations as it's…

0b0559f

… the default. Use containsString and hasPrefix instead of range checking.

Removed strong from property declaration because it's the default. Ad…

a94e354

…ded check for self in init.

General cleanup.

53b32d7

Removed strong from property declarations as it's the default. Remove…

eae40ce

…d confusing validate method and just check for nil/empty. No need to init to nil in Objective-C for objects.

Use self = [super init]; in case someone subclasses someday.

057c93c

General cleanup.

4036e42

General cleanup.

fe27688

General cleanup.

73a99ac

General cleanup.

58a9a16

No need to check for nil, a message sent in Objective-C to a nil rece…

81d6f14

…iver is a no-op.

No need for @synthesize. It's automatic for @Property.

c33de21

No need for @synthesize. It will be provided automatically.

81973bc

General cleanup.

92b8d40

General cleanup.

47d6703

Using the NS_UNAVAILABLE anotation, so no one can call the init metho…

ad956dc

…d. Other assorted minor cleanup.

Using the NS_UNAVAILABLE anotation, so no one can call the init metho…

5ac6b56

…d. Other assorted minor cleanup.

General cleanup.

ccf6c8c

baumatron mentioned this pull request May 11, 2017

Refine clang-format options and run on SDK source #213

Closed

baumatron suggested changes May 11, 2017

View reviewed changes

danieltiger added 3 commits May 11, 2017 13:55

Bring back iOS 8 checks for now.

62a5a30

Bring back selector check for now.

fb1cd2d

Remove major revision code.

d2af4c3

Revert the test.

3e9dd5a

baumatron approved these changes May 11, 2017

View reviewed changes

baumatron merged commit 2816c7c into keenlabs:master May 11, 2017

masojus mentioned this pull request May 16, 2017

Support for reading saved and cached query results #212

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General cleanup and Objective-C modernization #211

General cleanup and Objective-C modernization #211

danieltiger commented May 10, 2017 •

edited

coveralls commented May 10, 2017

baumatron left a comment

baumatron May 11, 2017

danieltiger May 11, 2017

baumatron May 11, 2017

baumatron May 11, 2017

baumatron May 11, 2017

danieltiger May 11, 2017

danieltiger May 11, 2017

baumatron May 11, 2017

baumatron May 11, 2017

danieltiger May 11, 2017

baumatron May 11, 2017

baumatron May 11, 2017

danieltiger May 11, 2017

baumatron May 11, 2017

danieltiger commented May 11, 2017

baumatron commented May 11, 2017

danieltiger commented May 11, 2017

coveralls commented May 11, 2017

baumatron commented May 11, 2017

danieltiger commented May 11, 2017

General cleanup and Objective-C modernization #211

General cleanup and Objective-C modernization #211

Conversation

danieltiger commented May 10, 2017 • edited

coveralls commented May 10, 2017

baumatron 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

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

danieltiger commented May 11, 2017

baumatron commented May 11, 2017

danieltiger commented May 11, 2017

coveralls commented May 11, 2017

baumatron commented May 11, 2017

danieltiger commented May 11, 2017

danieltiger commented May 10, 2017 •

edited