-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
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.
Looks great, especially with all the possible bugs you found! Interesting behavior. Would be interesting to see whether these are bugs in the rxjava-file-utils
library or in the underlying java.nio.*
package...
Here is some inline feedback. It's quite a bunch, but don't worry: some of them are complements as well 😜
import java.nio.file.Path | ||
import java.util | ||
|
||
import rx.Observer |
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 see you're currently using the RxJava library rather than the RxScala library. I'd encourage you to use the latter as much as possible. Given that the RxJavaFileUtils returns a RxJava Observable
, you have to change this to a RxScala Observable
. See section 2 for this transformation...
This is probably also why you have a class that extends Observer
, right? You can just do this with the three lambdas in Observable.subscribe
if you use RxScala. This does not work in Scala if you use the RxJava Observable
like you do now because Java lambdas and Scala lambdas do not yet match up. They will in Scala 2.12, though.
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.
Went through all kind of incompatible parameter issues with the onNext as in the java test class. Didn't retry after I got it working with extending the Observer class.
@@ -0,0 +1,71 @@ | |||
package workshop4.assignments |
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.
Would you mind moving this file to the assignments/solutions/RxEcosystem/
package and renaming it to the convention we used there?
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.
O, almost forgot: please add links to the README and the assignment description
val paths = Map((dir.toPath, FileSystemEventKind.values())) | ||
|
||
// what compiles | ||
val pathss = new util.HashMap[Path, Array[FileSystemEventKind]]() |
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 see you need a Java Map
here. You can use paths.asJava
here, after you imported scala.collection.JavaConverters._
.
|
||
val subscription = FileSystemWatcher | ||
.newBuilder() | ||
.addPaths(pathss) |
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.
Have you taken a look at the difference between addPaths
and addPath
?
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.
indeed I did, but Java's ... argument doesn't seem to accept a scala array
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.
🎉 🎉 🎉 Welcome to advanced Scala! 🎉 🎉 🎉
.addPath(dir.toPath, FileSystemEventKind.values():_*)
.withScheduler(Schedulers.io()) | ||
.build().subscribe(new FileSystemEventObserver) | ||
|
||
// run until receiving input (deamonize?), causes InterruptedException (when executed with IDE) |
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.
daemonize (spelling 😉 )
val subscription = FileSystemWatcher | ||
.newBuilder() | ||
.addPaths(pathss) | ||
.withScheduler(Schedulers.io()) |
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 like that you already looked at Scheduler
s a bit! That is one of the great features of Rx that we still have to cover later...
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.
just copy-pasted from the java test class in the project
// comes before processing the interrupt | ||
println("Received input, shutting down") | ||
|
||
subscription.unsubscribe() |
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.
Great to see that you thought of the unsubscribe
here! Many people (including me) forget this more than often...
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.
but it's not processed because of the interrupt
It's funny, but I get different behavior on my Windows computer... In your scenario "If a file exists on startup and gets changed after startup"
In your scenario "Deleting a file causes a modify of the folder and delete for the file.", I only get a DELETE event for the file itself, again with the incorrect path! Your scenario of "copy-pasting a large file into the folder under watch" gives the following:
All in all I think there are some serious differences between running the same (!!!) program on Mac and Windows! |
@jo-pol created an issue at ReactiveX/RxJavaFileUtils#11 |
to be discussed at the workshop