Refactor Content.rec(..) function to support tail-optimisation #417
Conversation
| * @return Result list of applying function to all files | ||
| */ | ||
| def recFilesApply[T]( | ||
| prefixPath: Path, |
There was a problem hiding this comment.
Could you try this if you are using intellij?
Setting => editor => coding style => scala
- formatter -> scalafmt
- configuration ->
dev\.scalafmt.conf
ctrl + alt + l
src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala
Outdated
Show resolved
Hide resolved
src/main/scala/com/microsoft/hyperspace/index/IndexLogEntry.scala
Outdated
Show resolved
Hide resolved
reduce visibility scope for recFilesApply Co-authored-by: EJ Song <[email protected]>
Co-authored-by: EJ Song <[email protected]>
| * @tparam T | ||
| * @return Result list of applying function to all files | ||
| */ | ||
| private[hyperspace] def recFilesApply[T]( |
There was a problem hiding this comment.
Could you move this function back to case class Content? as this is a private function that should be only used in case class Content.
Other than that, this change looks good to me :) Could you rebase this PR onto master?
There was a problem hiding this comment.
Hi, sorry for delay. I consider it as just static function, so it could live separately from class. It also simplify testing
There was a problem hiding this comment.
I'm fine with this being a generic utility function, but:
- The name
recFilesApplyseems unusual. MaybeapplyToFilesRecursively? - The meaning of "prefix" here seems unclear. What is the "root prefix" and what is the "current prefix"? Without looking at the code, it is hard to catch the meaning of the parameters.
There was a problem hiding this comment.
Thanks @clee704. Valid point, I have inherited prefixPath from original function. I have changed it to just path and added a short description why we need it
What is the context for this pull request?
What changes were proposed in this pull request?
Refactor Content.rec(..) function to support tail-optimisation
Does this PR introduce any user-facing change?
No
How was this patch tested?
Added unit tests