-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[camera] Remove OCMock from AvailableCamerasTests, CameraOrientationTests and ThreadSafeEventChannelTests #8363
base: main
Are you sure you want to change the base?
[camera] Remove OCMock from AvailableCamerasTests, CameraOrientationTests and ThreadSafeEventChannelTests #8363
Conversation
...foundation/ios/camera_avfoundation/Sources/camera_avfoundation/FLTCaptureDeviceControlling.m
Outdated
Show resolved
Hide resolved
@@ -1,3 +1,11 @@ | |||
## 0.9.17+8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't seen 2 released in a single PR before. This is probably not a standard practice (@stuartmorgan will know)
May worth splitting into 2 PRs, which can make the review easier/faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI won't allow this; it can either be two PRs, or it can be one PR with a single version bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a trick that you can set base branch your previous branch first, and after that gets landed, change the base branch back to main.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, but I believe that I can only set the base branch as a branch that exists in the main repo, it doesn't list branches from my fork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i will review after the previous PR is in then. otherwise it's hard to tell which part is new change.
error:(NSError **)error; | ||
@end | ||
|
||
@interface AVCaptureDevice (FLTCaptureDeviceControlling) <FLTCaptureDeviceControlling> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just call the protocol FLTCaptureDevice
since it's a noun (rather than a capability), as discussed in the design doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Follow-up on #8342. This should be merged after #8351
AvailableCamerasTests.m
,CameraOrientationTests.m
andThreadSafeEventChannelTests.m
FLTEventChannel
andFLTCameraDeviceDiscovering
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.