Skip to content
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

Add support for Apple Watch in CoreAardvark #41

Merged
merged 5 commits into from
Jan 12, 2017

Conversation

shawnwelch
Copy link
Collaborator

I think this should be all that's needed, but we'll have to see. Never tried making code that is shared between platforms watchos and ios

@dfed
Copy link
Collaborator

dfed commented Jan 11, 2017

Missing:

  • Bump to CoreAardvark pod spec. This likely should be a feature version bump
  • A way to get files off of the watch. If it's possible to share files between watch and iOS (I'd hope so), the best way to do this may be to allow ARKLogStore to have an injected URL rather than just a fileName. May want to add a initWithPersistedLogFileURL: initializer.

If there's no shared disk space between watch and iOS, then I guess it's up to the consumer to vend logs from the watch to the parent app.

@dfed dfed self-requested a review January 11, 2017 17:42
@shawnwelch
Copy link
Collaborator Author

Tracking getting logs from the watch with #42

Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

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

Looks like CoreAardvark needs some work to compile on WatchOS. Hopefully UIApplication* notifications have an equivalent on WatchOS.

s.license = 'Apache License, Version 2.0'
s.summary = 'Aardvark is a library that makes it dead simple to create actionable bug reports. Usable by extensions.'
s.homepage = 'https://github.com/square/Aardvark'
s.authors = 'Square'
s.source = { :git => 'https://github.com/square/Aardvark.git', :tag => "CoreAardvark/#{ s.version.to_s }" }
s.ios.deployment_target = '8.0'
s.watchos.deployment_target = '3.0'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Build is failing. Looks like watch doesn't know about UIApplicationWillTerminateNotification.

Copy link
Collaborator

@dfed dfed left a comment

Choose a reason for hiding this comment

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

Naming nits only. 🤞 for a green build!

@@ -57,4 +57,7 @@
/// Removes all logs. Completion handler is called on the main queue.
- (void)clearLogsWithCompletionHandler:(nullable dispatch_block_t)completionHandler;

/// Waits for all of the pending logs to distribute from the log distributor, then saves with optional wait. This is done automatically when platform=ios, but must be done manually in watchos, preferably in WKExtensionDelegate applicationWillResignActive;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's be clear as to when this is called on iOS.

@@ -128,12 +130,17 @@ - (void)clearLogsWithCompletionHandler:(nullable dispatch_block_t)completionHand
}
}

- (void)waitForPendingLogsThenSaveAndWait:(BOOL)wait;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how much the wait parameter gives us here, since we're already waiting for one thing. Maybe just waitUntilAllLogsConsumedAndArchiveSaved?

@dfed
Copy link
Collaborator

dfed commented Jan 11, 2017

Merge when green!

@shawnwelch shawnwelch merged commit 492fcb6 into master Jan 12, 2017
@dfed dfed deleted the shawnwelch/add-watch-3.0 branch January 12, 2017 16:53
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.

2 participants