Allow files for scala_import #411
Conversation
|
Can one of the admins verify this patch? |
|
Hi @jjudd, |
|
@jjudd gentle ping |
|
Thanks for the ping. I should be able to add a test for this in the next couple days. I got sidetracked with the Scala IntelliJ plugin :) |
|
Sure thing, thank you for your contributions!
…On Tue, Feb 6, 2018 at 8:50 AM James Judd ***@***.***> wrote:
Thanks for the ping. I should be able to add a test for this in the next
couple days. I got sidetracked with the Scala IntelliJ plugin :)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#411 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIF5LgjhLqNgUIVgYZ80Wj2RBi2wqRks5tR_Y9gaJpZM4RzabW>
.
|
|
I haven't forgotten about this. It's next on my list. |
|
@jjudd friendly ping |
This enables targets that are files to be used with scala_import. For example if you have a jar checked into your repository, you can use that with scala_import.
jjudd
force-pushed
the
files-scala-import
branch
from
c0a0d1c
to
4855d16
Compare
|
Alright. I took a crack at adding a test for this. The jar I checked in to the repo comes from https://github.com/lucidsoftware/relate on Maven: http://central.maven.org/maven2/com/lucidchart/relate_2.11/2.1.1/relate_2.11-2.1.1.jar I'm happy to use whatever jar people want. That was just the first one that came to my mine. |
|
this is great. thanks! |
| import org.specs2.mutable.SpecificationWithJUnit | ||
| import com.lucidchart.relate.SqlRow | ||
|
|
||
| class ScalaImportExposesJarsTest extends SpecificationWithJUnit { |
There was a problem hiding this comment.
@jjudd are you sure this runs/compiles? I don’t understand what target it belongs to?
There was a problem hiding this comment.
Nope. My bad. I copied it, but then decided I wanted to add it to the original test instead. I accidentally added it to this new file. I'll make a PR real quick.
There was a problem hiding this comment.
I have a commit that fixes this and am running tests locally to make sure things are good. I'll open a PR as soon as it is done. Sorry about that.
| class ScalaImportExposesJarsTest extends SpecificationWithJUnit { | ||
|
|
||
| "scala_import" should { | ||
| "enable using the jars it exposes" in { |
There was a problem hiding this comment.
Do we need this here? I think it was copied from another test and wasn’t removed right?
Hi everyone. Wanted to get people's thoughts on this before I added tests around it and prepared it to actually be merged.
I'm not sure if there is a reason the rules do not allow files for
scala_import. We made this change and have been using files fromscala_import. It also doesn't fail any tests locally.If this change is something everyone is ok with, then I'll add a test for it and get it ready for merging.