feat: add date fun facts (APP-61, APP-49) #32
Conversation
| + commit.raw?.shortMessage) | ||
| commit.repo = repo | ||
| commit | ||
| } |
There was a problem hiding this comment.
I think I miss why you cant' move commit.repo setting into .doOnNext, but actually not sure why you do commit.repo setting at all, I don't see where it's used
There was a problem hiding this comment.
Moved. Commit belong to repo. Used to retrieve repo rehash.
| factDayTime[dateTime.hour] += 1 | ||
| factsDayWeek[author] = factDayWeek | ||
| factsDayTime[author] = factDayTime | ||
| }, Observable.just(Commit())) |
There was a problem hiding this comment.
not part of this changeset, but just curious why do you need to attach a fake Commit() into the end?
There was a problem hiding this comment.
Commits are combined in pairs, an empty commit concatenated to calculate the diff of the initial commit.
|
|
||
| // Commits are combined in pairs, an empty commit concatenated to | ||
| // calculate the diff of the initial commit. | ||
| Observable.concat(getObservableCommits(), Observable.just(Commit())) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
getCommitsAsObservable(), ok.
| .doOnNext { commit -> | ||
| val author = commit.author | ||
| val factDayWeek = factsDayWeek[author] ?: Array(7) { 0 } | ||
| val factDayTime = factsDayTime[author] ?: Array(24) { 0 } |
There was a problem hiding this comment.
getOrPut should be nicer here
There was a problem hiding this comment.
Can't fit it in one line, the same logic inside, I'll leave it as is.
| postCommitsToServer(commitsBundle) | ||
| }, { e -> | ||
| Logger.error("Error while hashing: $e") | ||
| }, { |
There was a problem hiding this comment.
might be worth adding a comment that this is onComplete part
| testRepo.createFile("test1.txt", listOf("line1", "line2")) | ||
| testRepo.commit(message = "initial commit", | ||
| author = author1, | ||
| date = createDate(month = 1, day = 1, // Sunday. |
There was a problem hiding this comment.
might be worth to point year explicitly, if createDate() gets changed for some reason, then a failing test might take by surprise
| val knownCommits = repo.commits.toHashSet() | ||
|
|
||
| val factsDayWeek = hashMapOf<Author, Array<Int>>() | ||
| val factsDayTime = hashMapOf<Author, Array<Int>>() |
There was a problem hiding this comment.
btw, what's difference between hashMapOf and mutableMap?
There was a problem hiding this comment.
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.
| if (count > 0) { | ||
| facts.add(Fact(repo, FACT_KEY_COMMITS_DAYTIME + | ||
| hour, count.toDouble(), author)) | ||
| } |
There was a problem hiding this comment.
curious, if FACT_KEY_BLABLA_NUMBER can be sent to the server in a form to avoid strings parsing (regexps?), for example, if there's a way to serialize a list so that the server may deserialize it by some standard API.
| private val git: Git) { | ||
| companion object { | ||
| val FACT_KEY_COMMITS_DAYWEEK = "commits-dayweek-" | ||
| val FACT_KEY_COMMITS_DAYTIME = "commits-daytime-" |
There was a problem hiding this comment.
is it possible to put these into some namespace? it be nice to have something like FactKey.CommitDayWeek
* feat: add day of week and time of day fun facts (APP-49, APP-61) * wip: add account of time zones, switch to long type for timestamp * feat: test for date facts, add date to commit in TestRepo * fix: add FactKey for all key names, fix PR requests * chore: add year to test
No description provided.