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

Basic suggested migration #2385

Merged

Conversation

aperfilyev
Copy link
Collaborator

Screen.Recording.2021-05-10.at.15.32.19.mov

This only supports basic stuff from alter-table-stmt in sqlite like table rename and column add/delete/rename. The ui is not really customizable in the sdk. I had to delete FileViewProvider for query files because the indicator gets reset when another file changes. And I would really like your suggestions on how to generate migration scripts better.

@AlecKazakova
Copy link
Collaborator

HELL YEA will give this a thorough review shortly

Copy link
Member

@JakeWharton JakeWharton left a comment

Choose a reason for hiding this comment

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

Leaving to Alec to review the rest

My only concern is that someone may not see the subtle refactor icon

@aperfilyev aperfilyev force-pushed the aperfilyev/suggest-migration branch from d5bcc3a to dd1fbd5 Compare May 10, 2021 17:33
@AlecKazakova AlecKazakova mentioned this pull request Nov 23, 2021
12 tasks
@AlecKazakova AlecKazakova force-pushed the aperfilyev/suggest-migration branch 2 times, most recently from 3f48440 to 95016c7 Compare March 8, 2022 14:46
@AlecKazakova
Copy link
Collaborator

HELL YEA will give this a thorough review shortly

shortly has come

I'm working on this branch now, I've rebased it and got it running and working so I can step through stuff and provide better feedback

I think the SuggestedRefactoringSupport APIs aren't the right entryway to this UI for us. From what I can tell the editor keeps track of document changes and then we can suggest migration stuff based off of document diffs (ie the old User.sq state to the new User.sq state). What I think we want to do is have "old state" be a computation on an existing .db file + .sqm migrations. I'll keep looking through these APIs to see if something like that is possible via SuggestedRefactoringSupport but figured I'd comment to stay visible

@AlecKazakova
Copy link
Collaborator

yea this all looks really constrained to the signatureRange APIs, I suspect we will want our entryway to be more like the inspections you've made, though I like the UI this prompts you with so I wonder if we can keep that somehow 🤔

@AlecKazakova
Copy link
Collaborator

Also the file view provider needs to stay, or we have to change the file service. The file view provider was holding state about what that provider generated last time it ran so that we correctly delete files when the filename changes, but with the service it deletes all files except the currently edited one. I assume if we switch to use an Intention it will work fine with the provider again

@AlecKazakova
Copy link
Collaborator

Okay gave it a shot with the inspection APIs, I left the refactoring executor and signature builders that you wrote since those still worked well, and just used the inspection instead of the signature refactor entry point.

sqldelightMigrations.mov

import com.alecstrong.sql.psi.core.DialectPreset.SQLITE_3_24
import com.alecstrong.sql.psi.core.DialectPreset.SQLITE_3_25

interface SqlGeneratorStrategy {
Copy link
Collaborator

Choose a reason for hiding this comment

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

big fan of this API btw, nice

@AlecKazakova AlecKazakova merged commit 1e51f3f into cashapp:master Mar 9, 2022
@aperfilyev
Copy link
Collaborator Author

yo that's much better than my version

@aperfilyev aperfilyev deleted the aperfilyev/suggest-migration branch March 9, 2022 18:45
.joinToString(", ")

return """
|BEGIN TRANSACTION;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that changes with sqldelight 2.x? I was under the impression that at least for iOS and Android in 1.x world, you don't need transactions. There's also some documentation about it: https://cashapp.github.io/sqldelight/jvm_sqlite/migrations/

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea true theres no need for it

@vanniktech
Copy link
Contributor

Is there anything I need to do specifically? I'm using 2.0.0-alpha02 both runtime and the plugin but when adding a new column to a table I don't get the feature.

@AlecKazakova
Copy link
Collaborator

do you have a .db file and your dialect is SQLite? Those should be the only two requirements

@vanniktech
Copy link
Contributor

Yup:

Screen Shot 2022-04-17 at 18 18 50

sqldelight {
  QueryWrapper {
    packageName = "com.vanniktech.feature.rssreader"
    schemaOutputDirectory = file("src/commonMain/sqldelight/databases")
    verifyMigrations = true
  }
}

@AlecKazakova
Copy link
Collaborator

Can you try the snapshot version of the IDE? This change may have fixed it

@vanniktech
Copy link
Contributor

Indeed

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.

4 participants