Improved overflow support: Scaffolding#18
Improved overflow support: Scaffolding#18sungshik merged 10 commits intoimproved-overflow-support-mainfrom
Conversation
… parent watch to the constructor
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## improved-overflow-support-main #18 +/- ##
==================================================================
+ Coverage 80.0% 80.2% +0.2%
- Complexity 88 90 +2
==================================================================
Files 11 11
Lines 415 416 +1
Branches 41 41
==================================================================
+ Hits 332 334 +2
- Misses 57 59 +2
+ Partials 26 23 -3 ☔ View full report in Codecov by Sentry. |
… using the 3-arg constructor with a `null` arg)
sungshik
left a comment
sungshik
left a comment
There was a problem hiding this comment.
Clarifying comments
DavyLandman
left a comment
DavyLandman
left a comment
There was a problem hiding this comment.
Some minor things, but looks good
| var kind = translate(jdkKind); | ||
| var rootPath = path; | ||
| var relativePath = kind == WatchEvent.Kind.OVERFLOW ? Path.of("") : (@Nullable Path)jdkEvent.context(); | ||
| var relativePath = context == null ? Path.of("") : (Path) context; |
There was a problem hiding this comment.
this is now moved from the constructor of WatchEvent to the consumer, is that a win?
nit: looks like we could context inside of this definition.
var relativePath = jdkKind == StandardWatchEventKinds.OVERFLOW ? Path.of("") : (Path)jdkEvent.context();?
There was a problem hiding this comment.
is that a win?
Mja, maybe not. It seemed like a good idea at the time to get rid of the @Nullable and simplify the constructor a bit. But reading the consumer code now after the weekend, it's not so nice actually. I'm changing this back! (But keeping the 2-arg constructor in WatchEvent because I'll need it in the next PRs anyway.)
| assert !parent.equals(file); | ||
|
|
||
| this.internal = new JDKDirectoryWatch(parent, exec, e -> { | ||
| if (fileName.equals(e.getRelativePath())) { |
There was a problem hiding this comment.
what if an overflow happens for the whole directory, we would still need to deal with that event and mark this file as overflow? or is that bugfix happening later?
There was a problem hiding this comment.
Yes, I think this will/should be "magically" covered by one of the next PRs. But, we'll need a test to check this. I added a reminder for this to #12.
…WatchEvent` (because having it outside didn't lead to significantly simpler code overall)
2 participants
This PR makes a few changes to the existing code, originally made during the implementation of improved overflow support. I isolated them in this separate PR to make reviewing easier. (They would pollute the future improved-overflow-support PR.)