feat: add stats extractors, add commit diff calculation, add contribu... #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Merged

Parent: Master

astansler merged 3 commits into master from dev

Aug 14, 2017

Merged

feat: add stats extractors, add commit diff calculation, add contribu... #12

astansler merged 3 commits into master from dev

Aug 14, 2017

Conversation

@astansler

Copy link

Member

@astansler astansler commented Aug 13, 2017 •

edited

Loading

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

@astansler


 feat: add stats extractors, add commit diff calculation, add contribu...

acc8c27

...tors hashing filter

@astansler astansler self-assigned this

Aug 13, 2017

@astansler astansler requested review from asurkov and sergey48k

August 13, 2017 11:50

sergey48k

sergey48k requested changes

Aug 13, 2017

View reviewed changes

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.

src/main/kotlin/app/Main.kt Outdated

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

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

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

Ok.

src/main/kotlin/app/RepoHasher.kt Outdated

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

}

private fun findFirstOverlapCommit(): Commit? {

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

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

Ok.

src/main/kotlin/app/RepoHasher.kt

private fun findFirstOverlapCommit(): Commit? {

val serverHistoryCommits = repo.commits.toHashSet()

return getObservableCommits()

.skipWhile { commit -> !serverHistoryCommits.contains(commit) }

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

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

elegant!

src/main/kotlin/app/RepoHasher.kt

.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

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

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

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

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

okay, i guess...

src/main/kotlin/app/RepoHasher.kt Outdated

.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

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

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

Ok.

src/main/kotlin/app/RepoHasher.kt Outdated

val diffContents = getDiffContents(new, old)

Logger.debug("Commit: ${new.raw?.name ?: ""}: "

+ new.raw?.shortMessage)

Logger.debug("Diff: " + diffContents.joinToString("\n"))

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

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.

src/main/kotlin/app/extractors/JavaExtractor.kt

class JavaExtractor : ExtractorInterface {

val NAME = "Java"

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

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

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

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

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

okay, sounds good.

src/main/kotlin/app/model/DiffContent.kt

* Per file diff from commit.

data class DiffContent(

var path: Path = Paths.get(""),

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

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

Got it.

src/main/kotlin/app/utils/FileHelper.kt

object FileHelper {

fun getFileExtension(path: Path): String {

val fileName = path.fileName.toString()

return fileName.substringAfterLast(

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

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.

src/main/kotlin/app/utils/UiHelper.kt Outdated

package app.utils

object UiHelper {

fun confirm(message: String): Boolean {

Copy link

Member

@sergey48k sergey48k Aug 13, 2017

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

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

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

Got it.

@astansler


 fix: add default value to UiHelper.confirm, remove perfomance reducin...

fe0cf54

...g logging, fix stylistics

sergey48k

sergey48k approved these changes

Aug 14, 2017

View reviewed changes

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.

src/main/kotlin/app/utils/UiHelper.kt Outdated

fun confirm(message: String): Boolean {

println("$message [y/n]")

return (readLine() ?: "").toLowerCase() == "y"

fun confirm(message: String, default: Boolean): Boolean {

Copy link

Member

@sergey48k sergey48k Aug 14, 2017

Choose a reason for hiding this comment

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

make it explicit: defaultIsYes

src/main/kotlin/app/utils/UiHelper.kt Outdated

println("$message [y/n]")

return (readLine() ?: "").toLowerCase() == "y"

fun confirm(message: String, default: Boolean): Boolean {

val yes = "y"

Copy link

Member

@sergey48k sergey48k Aug 14, 2017

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


 chore: simplify UiHelper.confirm

a08dbad

@astansler astansler merged commit 7fbdf62 into master

Aug 14, 2017

@astansler astansler deleted the dev branch

August 14, 2017 09:50

Labels

None yet

2 participants

@astansler @sergey48k

Uh oh!

feat: add stats extractors, add commit diff calculation, add contribu... #12

feat: add stats extractors, add commit diff calculation, add contribu... #12

Uh oh!

Conversation

@astansler astansler commented Aug 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@sergey48k sergey48k left a comment

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 13, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k left a comment

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 14, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler commented Aug 13, 2017 •

edited

Loading