-
Notifications
You must be signed in to change notification settings - Fork 275
feat: add date fun facts (APP-61, APP-49) #32
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
Changes from all commits
115ce84
0a0b428
d20a1db
8bc6332
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package app | ||
|
|
||
| object FactKey { | ||
| val COMMITS_DAY_WEEK = "commits-day-week-" | ||
| val COMMITS_DAY_TIME = "commits-day-time-" | ||
| val LINE_LONGEVITY = "line-longevity-avg" | ||
| val LINE_LONGEVITY_REPO = "line-longevity-repo-avg" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,16 @@ | |
|
|
||
| package app.hashers | ||
|
|
||
| import app.FactKey | ||
| import app.Logger | ||
| import app.api.Api | ||
| import app.extractors.Extractor | ||
| import app.model.Author | ||
| import app.model.Commit | ||
| import app.model.DiffContent | ||
| import app.model.DiffFile | ||
| import app.model.DiffRange | ||
| import app.model.Fact | ||
| import app.model.LocalRepo | ||
| import app.model.Repo | ||
| import app.utils.RepoHelper | ||
|
|
@@ -26,6 +29,8 @@ import org.eclipse.jgit.lib.ObjectId | |
| import org.eclipse.jgit.errors.MissingObjectException | ||
| import org.eclipse.jgit.revwalk.RevCommit | ||
| import org.eclipse.jgit.util.io.DisabledOutputStream | ||
| import java.time.LocalDateTime | ||
| import java.time.ZoneOffset | ||
| import java.util.concurrent.TimeUnit | ||
|
|
||
| /** | ||
|
|
@@ -35,32 +40,52 @@ class CommitHasher(private val localRepo: LocalRepo, | |
| private val repo: Repo = Repo(), | ||
| private val api: Api, | ||
| private val git: Git) { | ||
|
|
||
| private val gitRepo: Repository = git.repository | ||
|
|
||
| private fun findFirstOverlappingCommit(): Commit? { | ||
| val serverHistoryCommits = repo.commits.toHashSet() | ||
| return getObservableCommits() | ||
| return getCommitsAsObservable() | ||
| .skipWhile { commit -> !serverHistoryCommits.contains(commit) } | ||
| .blockingFirst(null) | ||
| } | ||
|
|
||
| private fun hashAndSendCommits() { | ||
| val lastKnownCommit = repo.commits.lastOrNull() | ||
| val knownCommits = repo.commits.toHashSet() | ||
|
|
||
| val factsDayWeek = hashMapOf<Author, Array<Int>>() | ||
| val factsDayTime = hashMapOf<Author, Array<Int>>() | ||
|
|
||
| // Commits are combined in pairs, an empty commit concatenated to | ||
| // calculate the diff of the initial commit. | ||
| Observable.concat(getObservableCommits(), Observable.just(Commit())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. btw, not introduced by this changeset, so feel free to ignore it, but I would probably rename getObservableCommits() to commitsAsObservable() or something, getObservableCommits name is misleading, it makes me think that you want some sort of observable commits only, i.e. there are some not observable commits that we want to ignore.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. getCommitsAsObservable(), ok. |
||
| Observable.concat(getCommitsAsObservable() | ||
| .doOnNext { commit -> | ||
| Logger.debug("Commit: ${commit.raw?.name ?: ""}: " | ||
| + commit.raw?.shortMessage) | ||
| commit.repo = repo | ||
|
|
||
| // Calculate facts. | ||
| val author = commit.author | ||
| val factDayWeek = factsDayWeek[author] ?: Array(7) { 0 } | ||
| val factDayTime = factsDayTime[author] ?: Array(24) { 0 } | ||
| val timestamp = commit.dateTimestamp | ||
| val dateTime = LocalDateTime.ofEpochSecond(timestamp, 0, | ||
| ZoneOffset.ofTotalSeconds(commit.dateTimeZoneOffset * 60)) | ||
| // The value is numbered from 1 (Monday) to 7 (Sunday). | ||
| factDayWeek[dateTime.dayOfWeek.value - 1] += 1 | ||
| // Hour from 0 to 23. | ||
| factDayTime[dateTime.hour] += 1 | ||
| factsDayWeek[author] = factDayWeek | ||
| factsDayTime[author] = factDayTime | ||
| }, Observable.just(Commit())) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not part of this changeset, but just curious why do you need to attach a fake Commit() into the end?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| .pairWithNext() // Pair commits to get diff. | ||
| .takeWhile { (new, _) -> // Hash until last known commit. | ||
| new.rehash != lastKnownCommit?.rehash } | ||
| .filter { (new, _) -> knownCommits.isEmpty() // Don't hash known. | ||
| || !knownCommits.contains(new) } | ||
| .filter { (new, _) -> emailFilter(new) } // Email filtering. | ||
| .map { (new, old) -> // Mapping and stats extraction. | ||
| Logger.debug("Commit: ${new.raw?.name ?: ""}: " | ||
| + new.raw?.shortMessage) | ||
| new.repo = repo | ||
|
|
||
| val diffFiles = getDiffFiles(new, old) | ||
| Logger.debug("Diff: ${diffFiles.size} entries") | ||
| new.stats = Extractor().extract(diffFiles) | ||
|
|
@@ -73,18 +98,33 @@ class CommitHasher(private val localRepo: LocalRepo, | |
| total + file.getAllAdded().size } | ||
| new.numLinesDeleted = diffFiles.fold(0) { total, file -> | ||
| total + file.getAllDeleted().size } | ||
|
|
||
| new | ||
| } | ||
| .observeOn(Schedulers.io()) // Different thread for data sending. | ||
| .buffer(20, TimeUnit.SECONDS) // Group ready commits by time. | ||
| .doOnNext { commitsBundle -> // Send ready commits. | ||
| postCommitsToServer(commitsBundle) } | ||
| .blockingSubscribe({ | ||
| // OnNext | ||
| }, { t -> // OnError | ||
| Logger.error("Error while hashing: $t") | ||
| t.printStackTrace() | ||
| .blockingSubscribe({ commitsBundle -> // OnNext. | ||
| postCommitsToServer(commitsBundle) // Send ready commits. | ||
| }, { e -> // OnError. | ||
| Logger.error("Error while hashing: $e") | ||
| }, { // OnComplete. | ||
| val facts = mutableListOf<Fact>() | ||
| factsDayTime.map { (author, list) -> | ||
| list.forEachIndexed { hour, count -> | ||
| if (count > 0) { | ||
| facts.add(Fact(repo, FactKey.COMMITS_DAY_TIME + | ||
| hour, count.toDouble(), author)) | ||
| } | ||
| } | ||
| } | ||
| factsDayWeek.map { (author, list) -> | ||
| list.forEachIndexed { day, count -> | ||
| if (count > 0) { | ||
| facts.add(Fact(repo, FactKey.COMMITS_DAY_WEEK + | ||
| day, count.toDouble(), author)) | ||
| } | ||
| } | ||
| } | ||
| postFactsToServer(facts) | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -142,14 +182,21 @@ class CommitHasher(private val localRepo: LocalRepo, | |
| } | ||
| } | ||
|
|
||
| private fun postFactsToServer(facts: List<Fact>) { | ||
| if (facts.isNotEmpty()) { | ||
| api.postFacts(facts) | ||
| Logger.debug("Sent ${facts.size} facts to server") | ||
| } | ||
| } | ||
|
|
||
| private fun deleteCommitsOnServer(commits: List<Commit>) { | ||
| if (commits.isNotEmpty()) { | ||
| api.deleteCommits(commits) | ||
| Logger.debug("Sent ${commits.size} deleted commits to server") | ||
| } | ||
| } | ||
|
|
||
| private fun getObservableCommits(): Observable<Commit> = | ||
| private fun getCommitsAsObservable(): Observable<Commit> = | ||
| Observable.create { subscriber -> | ||
| try { | ||
| val revWalk = RevWalk(gitRepo) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
|
|
||
| package test.tests.hashers | ||
|
|
||
| import app.FactKey | ||
| import app.api.MockApi | ||
| import app.hashers.CommitHasher | ||
| import app.model.* | ||
|
|
@@ -12,11 +13,14 @@ import org.eclipse.jgit.api.Git | |
| import org.jetbrains.spek.api.Spek | ||
| import org.jetbrains.spek.api.dsl.given | ||
| import org.jetbrains.spek.api.dsl.it | ||
| import test.utils.TestRepo | ||
| import java.io.File | ||
| import java.util.* | ||
| import java.util.stream.StreamSupport.stream | ||
| import kotlin.streams.toList | ||
| import kotlin.test.assertEquals | ||
| import kotlin.test.assertNotEquals | ||
| import kotlin.test.assertTrue | ||
|
|
||
| class CommitHasherTest : Spek({ | ||
| val userName = "Contributor" | ||
|
|
@@ -56,6 +60,14 @@ class CommitHasherTest : Spek({ | |
| return lastCommit | ||
| } | ||
|
|
||
| fun createDate(year: Int = 2017, month: Int = 1, day: Int = 1, | ||
| hour: Int = 0, minute: Int = 0, seconds: Int = 0): Date { | ||
| val cal = Calendar.getInstance() | ||
| // Month in calendar is 0-based. | ||
| cal.set(year, month - 1, day, hour, minute, seconds) | ||
| return cal.time | ||
| } | ||
|
|
||
| given("repo with initial commit and no history") { | ||
| repo.commits = listOf() | ||
|
|
||
|
|
@@ -199,5 +211,66 @@ class CommitHasherTest : Spek({ | |
| } | ||
| } | ||
|
|
||
| given("test of commits date facts") { | ||
| val testRepoPath = "../testrepo1" | ||
| val testRepo = TestRepo(testRepoPath) | ||
| val author1 = Author("Test1", "test@domain.com") | ||
| val author2 = Author("Test2", "test@domain.com") | ||
|
|
||
| val repo = Repo(rehash = "rehash", commits = listOf()) | ||
| val mockApi = MockApi(mockRepo = repo) | ||
| val facts = mockApi.receivedFacts | ||
|
|
||
| afterEachTest { | ||
| facts.clear() | ||
| } | ||
|
|
||
| it("send two facts") { | ||
| testRepo.createFile("test1.txt", listOf("line1", "line2")) | ||
| testRepo.commit(message = "initial commit", | ||
| author = author1, | ||
| date = createDate(month = 1, day = 1, // Sunday. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. might be worth to point year explicitly, if createDate() gets changed for some reason, then a failing test might take by surprise
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. |
||
| hour = 13, minute = 0, seconds = 0)) | ||
|
|
||
| CommitHasher(localRepo, repo, mockApi, testRepo.git).update() | ||
|
|
||
| assertEquals(2, facts.size) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_TIME + 13, | ||
| 1.0, author1))) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_WEEK + 6, | ||
| 1.0, author1))) | ||
| } | ||
|
|
||
| it("send more facts") { | ||
| testRepo.createFile("test2.txt", listOf("line1", "line2")) | ||
| testRepo.commit(message = "second commit", | ||
| author = author2, | ||
| date = createDate(month = 1, day = 2, // Monday. | ||
| hour = 18, minute = 0, seconds = 0)) | ||
|
|
||
| testRepo.createFile("test3.txt", listOf("line1", "line2")) | ||
| testRepo.commit(message = "third commit", | ||
| author = author1, | ||
| date = createDate(month = 1, day = 2, // Monday. | ||
| hour = 13, minute = 0, seconds = 0)) | ||
|
|
||
| CommitHasher(localRepo, repo, mockApi, testRepo.git).update() | ||
|
|
||
| assertEquals(5, facts.size) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_TIME + 18, | ||
| 1.0, author2))) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_WEEK + 0, | ||
| 1.0, author2))) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_TIME + 13, | ||
| 2.0, author1))) | ||
| assertTrue(facts.contains(Fact(repo, FactKey.COMMITS_DAY_WEEK + 0, | ||
| 1.0, author1))) | ||
| } | ||
|
|
||
| afterGroup { | ||
| testRepo.destroy() | ||
| } | ||
| } | ||
|
|
||
| Runtime.getRuntime().exec("src/test/delete_repo.sh").waitFor() | ||
| }) | ||
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.
btw, what's difference between hashMapOf and mutableMap?
Uh oh!
There was an error while loading. Please reload this page.
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.
MutableMap is an interface and mutableMapOf create default implementation of map, which is for kotlin 1.1.4-2 LinkedHashMap.
hashMapOf and linkedMapOf clearly create HashMap and LinkedHashMap which implement MutableMap interface.