[ SI-7514 ] Introduce Source.fromResource(...) method #4561
Conversation
|
This test is flaky: |
|
@som-snytt I assume that this failing test has nothing related to regression caused by my PR? |
|
/rebuild |
|
Seems it's in a good shape now. |
|
For the record, the build issues turned out to be a hotspot bug (see #4578 (comment)). |
|
|
||
| def fromClassPath(resource: String, classLdr: ClassLoader = ClassLoader.getSystemClassLoader)(implicit codec: Codec): BufferedSource = { | ||
| val is = classLdr.getResourceAsStream(resource) | ||
| createBufferedSource(is, reset = () => fromInputStream(is)(codec), close = () => is.close())(codec) |
There was a problem hiding this comment.
Couldn't this be implemented more simply as fromInputStream(classLdr.getResourceAsStream(resource))?
There was a problem hiding this comment.
Ping @jdevelop. I'd like to avoid code duplication here, unless there's some compelling performance reason.
There was a problem hiding this comment.
@adriaanm Updated, pls take a look. Thanks!
|
(almost -- see comments below) LGTM -- thanks for your contribution and the informative commit message! |
|
|
||
| def fromClassPath(resource: String, classLdr: ClassLoader = ClassLoader.getSystemClassLoader)(implicit codec: Codec): BufferedSource = { | ||
| fromInputStream(classLdr.getResourceAsStream(resource))(codec) | ||
| } |
There was a problem hiding this comment.
Extraneous brace alert! See previous line. I'm surprised it passes muster with Jenkins. He is very particular.
There was a problem hiding this comment.
if it's in conflict with the current code style - I'll remove that. But as recent discoveries prove, a missing brace could cause some .. undesired behavior ;)
There was a problem hiding this comment.
I wouldn't put that in braces, but I didn't want to cause friction here, since we hadn't been as prompt with our review as I'd have liked.
There was a problem hiding this comment.
@adriaanm @som-snytt No problem with that, I removed the braces. :)
|
Gentle reminder that we're cutting M2 this week, hopefully on Wednesday (sorry for the rush -- we're already running a bit behind schedule). If you get a chance, please update this PR so we can still consider it for inclusion. Otherwise, we'll have to move it to M3. |
|
@adriaanm @SethTisue @som-snytt @dragos pls take a look Thanks! |
|
LGTM. Thanks for the quick turnaround! |
|
LGTM |
| @@ -28,6 +28,10 @@ class SourceTest { | |||
| @Test def canIterateLines() = { | |||
| assertEquals(sampler.lines.size, (Source fromString sampler).getLines.size) | |||
| } | |||
| @Test def loadFromClassPath() = { | |||
There was a problem hiding this comment.
test name should be updated since the method name changed
|
I'm going to ask for one more change on this: the new method should have Scaladoc. Other than that (and the test name, see comment above) I'm ready to hit "merge". |
Creates Source from named classpath resource.
Simplifies
val src = io.Source.fromInputStream(classOf[ClassInst].getResourceAsStream("/name"))
to
val src = io.Source.fromClassPath("/name")
|
@SethTisue updated the test name, added some JavaDocs. Pease take a look. |
|
By the end of this PR, jdevelop will have lost about 10 kg. I think I saw SethTisue in a movie about boot camp. You will take this tooth brush and make this PR shine so I can see my reflection, is that clear, soldier? Yessir, yessir! |
SI-7514 Introduce Source.fromResource(...) method
|
cue 🎆! |
|
thank you @jdevelop!! |
|
And of course there's a twist (or a tweest?) http://stackoverflow.com/questions/3121449/getclass-getclassloader-getresourceasstream-is-caching-the-resource (suggested on Twitter by @carlfish) |
|
Yeah, it's one of those nasty classloader wrinkles that nobody realises until you discover 50-100MB of your application footprint is made up of plugin files you loaded once, but are being held in memory forever. :) |
|
It does look like |
|
On further investigation, it seems this caching only happens in certain non-core classloaders, like the one in Tomcat web servers. Since this makes it somebody else's bad implementation decision, it might be worth ignoring after all. |
|
Ok, thanks for looking into it! |
Creates Source from named classpath resource.
Simplifies
val src = io.Source.fromInputStream(classOf[ClassInst].getResourceAsStream("/name"))
to
val src = io.Source.fromClassPath("/name")