-
Notifications
You must be signed in to change notification settings - Fork 115
Refactor Content.rec(..) function to support tail-optimisation #417
base: master
Are you sure you want to change the base?
Conversation
* @return Result list of applying function to all files | ||
*/ | ||
def recFilesApply[T]( | ||
prefixPath: Path, |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with this being a generic utility function, but:
- The name
recFilesApply
seems 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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