-
Notifications
You must be signed in to change notification settings - Fork 904
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
feat: add platform-cli-apple with reusable utilities for OOT platforms #2208
feat: add platform-cli-apple with reusable utilities for OOT platforms #2208
Conversation
packages/cli-platform-ios/src/commands/buildIOS/buildProject.ts
Outdated
Show resolved
Hide resolved
98fa97f
to
67ec2ff
Compare
283eac4
to
d572c25
Compare
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.
left some comments
packages/cli-platform-apple/src/commands/logCommand/createLog.ts
Outdated
Show resolved
Hide resolved
packages/cli-platform-apple/src/commands/runCommand/createRun.ts
Outdated
Show resolved
Hide resolved
packages/cli-platform-apple/src/commands/runCommand/createRun.ts
Outdated
Show resolved
Hide resolved
packages/cli-platform-apple/src/commands/runCommand/getSDKNameFromPlatform.ts
Outdated
Show resolved
Hide resolved
89e7074
to
e19db37
Compare
e19db37
to
1d4d80e
Compare
packages/cli-platform-apple/src/commands/buildCommand/buildProject.ts
Outdated
Show resolved
Hide resolved
export function getFallbackSimulator(args: FlagsT): Device { | ||
/** | ||
* If provided simulator does not exist, try simulators in following order | ||
* - iPhone 14 | ||
* - iPhone 13 | ||
* - iPhone 12 | ||
* - iPhone 11 | ||
*/ |
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.
Not really applicable for all apple platforms
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.
Yeah, it's used only on iOS - I don't want to remove it to introduce breaking changes
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.
Let's create a new issue to remove this
@@ -7,7 +7,8 @@ | |||
*/ | |||
|
|||
import execa from 'execa'; | |||
import listIOSDevices from '../listIOSDevices'; | |||
import listDevices from '../listDevices'; | |||
import {getPlatformInfo} from '../../commands/runCommand/getPlatformInfo'; |
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.
Looks like this method is here by accident. Can we create a dedicated commands/runCommand/__tests__/getPlatformInfo.test.ts
?
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.
Left a few small things to address, overall LGTM and nice refactor
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 work! 🎉
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.
please check failing unit tests 👀
Summary:
The goal of this PR is to refactor most of
cli-platform-ios
tocli-platform-apple
which provides reusable utilities allowing OOT platforms to build their commands. Example:Test Plan:
CI Green
Checklist