Refactor common features of JDK...Watcher into common JDKBaseWatch
#13
Conversation
…veWatch` and distinguish from existing `Watcher`)
…s (because it's a more general utility)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
=========================================
+ Coverage 78.1% 80.2% +2.0%
- Complexity 84 89 +5
=========================================
Files 10 11 +1
Lines 421 415 -6
Branches 44 41 -3
=========================================
+ Hits 329 333 +4
+ Misses 65 57 -8
+ Partials 27 25 -2 ☔ View full report in Codecov by Sentry. |
sungshik
left a comment
sungshik
left a comment
There was a problem hiding this comment.
A few clarifying comments
src/main/java/engineering/swat/watch/impl/jdk/JDKRecursiveDirectoryWatch.java
Outdated
Show resolved
Hide resolved
DavyLandman
left a comment
DavyLandman
left a comment
There was a problem hiding this comment.
LGTM, a few small nits. Thanks for refactoring this into a base class that shares more of the similar code.
| * @throws IOException When an I/O exception of some sort (e.g., a nested | ||
| * watch failed to start) has occurred | ||
| */ | ||
| protected abstract boolean runIfFirstTime() throws IOException; |
There was a problem hiding this comment.
I like the idea, but as an implementer I'm not so sure who should check if it's the first time? I originally thought this base class would take care, but it looks like it doesn't? Why don't we keep track of this fact in this class?
There was a problem hiding this comment.
Yes, that's cleaner. Fixed.
| throw new IllegalArgumentException("Unexpected watch event: " + jdkEvent); | ||
| } | ||
| var rootPath = path; | ||
| var relativePath = kind == WatchEvent.Kind.OVERFLOW ? rootPath : (@Nullable Path)jdkEvent.context(); |
There was a problem hiding this comment.
how come rootPath is also a valid relativePath? should it not be an empty path?
There was a problem hiding this comment.
Agreed, that makes more sense. Fixed.
| throw new IOException("Could not register file watcher for: " + file, e); | ||
| // Use local variables to check null before field assignments (Checker | ||
| // Framework doesn't like it the other way around) | ||
| var parent = path.getParent(); |
There was a problem hiding this comment.
alternative is to use Objects::requireNonNull as that makes the code a bit more compact (and CF correctly models the guarantees of requireNonNull
There was a problem hiding this comment.
Objects.requireNonNull doesn't work well with CF here because it still throws an NPE. I like the idea of having a small function to do the heavy lifting (well, not that heavy...), though, so I added a requireNonNull for Paths that throws an IllegalArgumentException. It makes the constructor more compact.
| throw new IllegalArgumentException("Unexpected watch event: " + jdkEvent); | ||
| } | ||
| var rootPath = path; | ||
| var relativePath = kind == WatchEvent.Kind.OVERFLOW ? rootPath : (@Nullable Path)jdkEvent.context(); |
There was a problem hiding this comment.
Agreed, that makes more sense. Fixed.
| throw new IOException("Could not register file watcher for: " + file, e); | ||
| // Use local variables to check null before field assignments (Checker | ||
| // Framework doesn't like it the other way around) | ||
| var parent = path.getParent(); |
There was a problem hiding this comment.
Objects.requireNonNull doesn't work well with CF here because it still throws an NPE. I like the idea of having a small function to do the heavy lifting (well, not that heavy...), though, so I added a requireNonNull for Paths that throws an IllegalArgumentException. It makes the constructor more compact.
| * @throws IOException When an I/O exception of some sort (e.g., a nested | ||
| * watch failed to start) has occurred | ||
| */ | ||
| protected abstract boolean runIfFirstTime() throws IOException; |
There was a problem hiding this comment.
Yes, that's cleaner. Fixed.
2 participants
In preparation of work to improve support for overflows (#12), this PR refactors some common features of the existing
JDK...Watchclasses out of those classes into a shared base class. This PR is supposed to be purely refactoring, so it's intended to leave the code functionally exactly the same.The commit messages should give a good overview of the changes.