wip: separate RepoHasher and CommitHasher, basic integration of CodeL... #22

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

astansler merged 2 commits into master from dev

Aug 30, 2017

Merged

wip: separate RepoHasher and CommitHasher, basic integration of CodeL... #22

astansler merged 2 commits into master from dev

Aug 30, 2017

Conversation

@astansler

Copy link

Member

@astansler astansler commented Aug 30, 2017

...ongevity

@astansler


 wip: separate RepoHasher and CommitHasher, basic integration of CodeL...

84756b5

...ongevity

@astansler astansler self-assigned this

Aug 30, 2017

@astansler astansler requested review from sergey48k and yandzee

August 30, 2017 09:56

sergey48k

sergey48k requested changes

Aug 30, 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.

Looks very good overall, but I want some changes.

src/main/kotlin/app/hashers/CodeLongevity.kt

val author = line.from.commit.getAuthorIdent()

if (author.getEmailAddress() !=email) {

val email = line.from.commit.authorIdent.emailAddress

if (!emails.contains(email)) {

Copy link

Member

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

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

We also need to compute average for entire repo. Remember the chart?

Copy link

Member Author

@astansler astansler Aug 30, 2017

Choose a reason for hiding this comment

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

Will be in next pr very-very soon.

src/main/kotlin/app/hashers/CodeLongevity.kt

// If a file was deleted, then the new path is /dev/null.

val path = if (newPath != DiffEntry.DEV_NULL) newPath else oldPath

var lines = files.get(path)!!

val path = if (newPath != DiffEntry.DEV_NULL) {

Copy link

Member

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

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

why did this change? the original looked more compact.

Copy link

Member Author

@astansler astansler Aug 30, 2017

Choose a reason for hiding this comment

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

Doesn't fit 80 limit.

src/main/kotlin/app/hashers/CodeLongevity.kt

lines.addAll(delStart, tmpLines)

// TODO(alex): Approve code change.

// delStart index caused IndexOutOfBoundException.

lines.addAll(tmpLines)

Copy link

Member

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

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

We really need unit tests, right @asurkov ?

Copy link

Contributor

@asurkov asurkov Sep 6, 2017

Choose a reason for hiding this comment

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

Absolutely. Btw, I always though that TODO(name) is about who added a todo item, not a person who's supposed to fix it.

Copy link

Member Author

@astansler astansler Sep 6, 2017

Choose a reason for hiding this comment

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

I think of this as a way to notify a person. Is there any conventions on that?

src/main/kotlin/app/hashers/CommitHasher.kt Outdated

deleteCommitsOnServer(deletedCommits)

}

// Delete missing local commits. If found at least one common commit

Copy link

Member

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

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

This sounds confusing. You are deleting commits on server, not locally.

Copy link

Member Author

@astansler astansler Aug 30, 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/hashers/RepoHasher.kt

println("Hashing $localRepo...")

val git = loadGit(localRepo.path)

try {

localRepo.parseGitConfig(git.repository.config)

Copy link

Member

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

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

I really like how neat this function is.

@astansler


 chore: comments

@astansler astansler merged commit 2e9dc37 into master

Aug 30, 2017

@astansler astansler deleted the dev branch

August 30, 2017 18:32

Labels

None yet

4 participants

@astansler @asurkov @sergey48k

Uh oh!

wip: separate RepoHasher and CommitHasher, basic integration of CodeL... #22

wip: separate RepoHasher and CommitHasher, basic integration of CodeL... #22

Uh oh!

Conversation

@astansler astansler commented Aug 30, 2017

Uh oh!

@sergey48k sergey48k left a comment

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@asurkov asurkov Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Sep 6, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@astansler astansler Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

@sergey48k sergey48k Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants