-
Notifications
You must be signed in to change notification settings - Fork 33
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
First phase of react-native-ide to radon IDE renaming - renaming non-visible bits first #538
Conversation
…visible items first
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 inline comments, please take a look at them before merging
"description": "Provides a way to customize Gradle builds for Android", | ||
"type": "object", | ||
"properties": { | ||
"variant": { |
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.
Those are incorrect properties, android launch configuration takes two values, buildType
and productFlavor
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 just copied schema from the config we currently use. I can change that in both places then
@@ -25,7 +25,9 @@ export class LaunchConfigController implements Disposable, LaunchConfig { | |||
|
|||
const configurations = launchConfiguration.get<Array<Record<string, any>>>("configurations")!; | |||
|
|||
const RNIDEConfiguration = configurations.find(({ type }) => type === "react-native-ide"); | |||
const RNIDEConfiguration = configurations.find( | |||
({ type }) => type === "react-native-ide" || type === "radon-ide" // for compatibility we want to support old configuration type name |
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.
This solution will give a first instance of a configuration, so if someone has two configurations at the same time they will get a random one, I believe we should put a priority on "radon-ide" configurations.
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 don't intent to keep them both actually. The plan is that in the follow up version we will be displaying messages if someone is using the old config asking to rename it. Would prefer to avoid adding complexity to handle a usecase that seem to be very unlikely
…d during the review
This is the first step for our renaming process. It renames most react-native-ide occurences that doesn't affect users or are not visible.
Final tricky part is caches file location. Since it contains android emulator configs that in turn contain absolute path names we may need to consider dropping caches entirely with the update (haven't decided on this one yet).
Test plan