Skip to content

Conversation

@astansler
Copy link
Member

@astansler astansler commented Aug 13, 2017

…tors hashing filter (APP-5, APP-6)

@astansler astansler self-assigned this Aug 13, 2017
@astansler astansler requested review from asurkov and sergey48k August 13, 2017 11:50
Copy link
Member

@sergey48k sergey48k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This overall looks good but it has some puzzling and some unexpected changes. I would like to at least see some explanations. Also, see minor comments re. formatting and usability.

if (!Configurator.isFirstLaunch()) {
println("Are you sure that you want to setup Sourcerer again? [y/n]")
if ((readLine() ?: "").toLowerCase() == "y") {
if(UiHelper.confirm("Are you sure that you want to setup Sourcerer "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add space after 'if'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

.find { it.rehash == repo.rehash } != null
}

private fun findFirstOverlapCommit(): Commit? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overlapping

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

private fun findFirstOverlapCommit(): Commit? {
val serverHistoryCommits = repo.commits.toHashSet()
return getObservableCommits()
.skipWhile { commit -> !serverHistoryCommits.contains(commit) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elegant!

.pairWithNext() // Pair commits to get diff.
.takeWhile { (new, _) -> // Hash until last requested commit.
new.rehash != lastKnownCommit?.rehash }
.filter { (new, _) -> knownCommits.isEmpty() // Don't hash known.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pet peeve: can you move || to the prev line? This helps the flow a little.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convention of wrapping lines before non-assignment operators are used. Moving || will break consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, i guess...

.filter { (new, _) -> knownCommits.isEmpty() // Don't hash known.
|| !knownCommits.contains(new) }
.filter { (new, _) -> emailFilter(new) } // Email filtering.
.map { (new, old) -> // Mapping and stats extracting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...stats extraction.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

val diffContents = getDiffContents(new, old)
Logger.debug("Commit: ${new.raw?.name ?: ""}: "
+ new.raw?.shortMessage)
Logger.debug("Diff: " + diffContents.joinToString("\n"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance this will affect performance? even if log level is above debug, it will do the string contact operation for all diffs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes. I'll leave printing of only first entry.

class JavaExtractor : ExtractorInterface {
val NAME = "Java"

val KEYWORDS = listOf("abstract", "continue", "for", "new", "switch",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ummm... why are you doing this? We did not make a product decision to parse out keywords. We discussed it as one of several ideas, and while it'a neat idea, we need to have Ryan's buy-in for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more for testing purposes. It isn't accurate and it was easiest feature to implement for more complex stats than just language stats.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, sounds good.

* Per file diff from commit.
*/
data class DiffContent(
var path: Path = Paths.get(""),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now, but in future, we will need to track more info so that extractors could make better decisions. For instance, antlr will need an entire file to parse out, so the extractor will need line numbers that we were added or deleted, not the lines themselves.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

object FileHelper {
fun getFileExtension(path: Path): String {
val fileName = path.fileName.toString()
return fileName.substringAfterLast(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to believe there isn't a standard java library that does that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me too. I could add apache commons io for this simple function, but it looks like overkill.

package app.utils

object UiHelper {
fun confirm(message: String): Boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's usually a nice idea to require precisely 'y' or 'n', and if the input is something else, it makes sense to repeat the question. An alternative is to have a default (configurable through the call to UIHelper.confirm) that will capitalize the default choice e.g. "y/N"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

Copy link
Member

@sergey48k sergey48k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but address comments before pushing.

fun confirm(message: String): Boolean {
println("$message [y/n]")
return (readLine() ?: "").toLowerCase() == "y"
fun confirm(message: String, default: Boolean): Boolean {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make it explicit: defaultIsYes

println("$message [y/n]")
return (readLine() ?: "").toLowerCase() == "y"
fun confirm(message: String, default: Boolean): Boolean {
val yes = "y"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be simplified to:

val yes = "Y" if defaultIsYes else "y";
val no = "N" if !defaultIsYes else "n";
println("$message [$yes/$no]);

@astansler astansler merged commit 7fbdf62 into master Aug 14, 2017
@astansler astansler deleted the dev branch August 14, 2017 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants