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

Remove hardcoded imports in slick migrations #22

Merged

Conversation

kshepard
Copy link
Contributor

@kshepard kshepard commented Aug 2, 2016

When adding a new slick migration, the generated code always contains imports for Users and UserRow. This commit removes those hardcoded import statements.

@lastland
Copy link
Owner

lastland commented Aug 4, 2016

Thanks for the contribution! I'm currently traveling. I will take a look at it as soon as I find some time.

On Aug 3, 2016, at 1:25 AM, Kenny Shepard notifications@github.com wrote:

When adding a new slick migration, the generated code always contains imports for Users and UserRow. This commit removes those hardcoded import statements.

Note: I have not run the unit tests, but if they for some reason rely on these hardcoded imports, they would also need to be adjusted.

You can view, comment on, or merge this pull request online at:

#22

Commit Summary

Remove hardcoded imports in slick migrations
File Changes

M migrations/slick/src/main/scala/Commands.scala (9)
Patch Links:

https://github.com/lastland/scala-forklift/pull/22.patch
https://github.com/lastland/scala-forklift/pull/22.diff

You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

import datamodel.v${version - 1}.schema.tables.UsersRow"""
else ""
s"""import ${driverName}.api._
case DBIO => s"""import ${driverName}.api._
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this would be a good idea, because I think in a Slick migration, the code usually relies on some object of the former version (in this case Users and UserRow). Even though users can simply add these imports manually, having these imports in the template can save users quite some trouble, and remind users to always import the predecessor version of the generated code instead of the latest version or one of the other former versions. Therefore, I would like to keep these imports in the template.

However, it is indeed not appropriate to have Users and UsersRow in it. What do you think if we use the wildcard import style (i.e. import datamodel.v${version - 1}.schema.tables._)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely agreed, wildcard imports should work well, I'll modify the code.

@lastland
Copy link
Owner

lastland commented Aug 4, 2016

This also reminds me that I may need to add some tests to this and similar cases. Probably gonna add a subproject-style test and use replace to modify the content in the generated migration file. I will work on this maybe a few days later. In the meantime, you are welcome if you want to add some tests for this pull request.

@kshepard kshepard force-pushed the feature/remove-hardcoded-imports branch from f076f4b to ee78566 Compare August 4, 2016 13:12
@kshepard
Copy link
Contributor Author

kshepard commented Aug 4, 2016

Alright, I updated the code to use a wildcard import. I also made it use pkgName for constructing the import, since that is what defines the package name and may be overridden. Unfortunately I'm not going to have time to write tests for this at the moment, but I tested it manually and it works.

@lastland lastland merged commit 69aea0f into lastland:develop Aug 4, 2016
@lastland
Copy link
Owner

lastland commented Aug 4, 2016

That's awesome! Thanks!

lastland added a commit that referenced this pull request Aug 9, 2016
lastland added a commit that referenced this pull request Aug 9, 2016
lastland added a commit that referenced this pull request Aug 9, 2016
lastland added a commit that referenced this pull request Aug 9, 2016
karolchmist pushed a commit to karolchmist/scala-forklift that referenced this pull request Apr 24, 2020
…-imports

Remove hardcoded imports in slick migrations
karolchmist pushed a commit to karolchmist/scala-forklift that referenced this pull request Apr 24, 2020
karolchmist pushed a commit to karolchmist/scala-forklift that referenced this pull request Apr 24, 2020
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