Register report: group by #122
Conversation
Register report grouping is based on year, month, day, isoWeek or isoWeekDate. This can be selected with "groupBy" config-option. Default is month, so if no groupBy option is defined, then behaviour is unchanged. This also changes Date.isoWeekDay -> isoWeekDate which is correct term for ISO 8601 weekdate.
Use correct val names so they are not misleading (posts vs. txns), for now this is done only in scope of groupBy.
|
|
||
| /** | ||
| * Grouping is done by truncated int numbers. The end result (groups) is | ||
| * Seq[(String, Seq[PostGroup])] so we need mapping from truncated intDate -> String |
There was a problem hiding this comment.
Hmm, what if we use strings through out for grouping? The integer values would be converted to a string before the grouping.
There was a problem hiding this comment.
That's one option. However, probably in most of the cases, sorting is done by year, month or day. By doing sorting in these cases as Int should be much faster than purely string based sorting. However, I don't have any specific numbers. And it was kind of interesting to build that bifurcation. =)
| } | ||
| } | ||
|
|
||
| case class RegisterReportSettings(_title: String, _accountMatch: Option[Seq[String]], _outFiles: Seq[String], groupBy: GroupBy) extends ReportSettings(_title, _accountMatch, _outFiles) { |
There was a problem hiding this comment.
minor: private vals are being prefixed with _. It would be good to rename groupBy to _groupBy just for convention sake.
There was a problem hiding this comment.
Ack, will fix this one!
| sealed case class GroupByMonth() extends GroupBy | ||
| sealed case class GroupByDay() extends GroupBy | ||
| sealed case class GroupByIsoWeek() extends GroupBy | ||
| sealed case class GroupByIsoWeekDate() extends GroupBy |
There was a problem hiding this comment.
You could consider using object instead of case class. For example:
object GroupByYear extends GroupBy
There was a problem hiding this comment.
Later I like to use these as closed set enums, so that compiler can do exhaustive check in matchers. With objects that's not possible?
Especially here: https://github.com/hrj/abandon/pull/122/files#diff-4943d1460731e0560df4dd1a2f3a6088R444
Throw:n exception while figuring out user input is kind of ok (input/cfg validation), but later inside maps etc. is is not so nice. That was the reason behind sealed case class.
Use '_'-prefix for private variables.
|
Hi, here is update PR. I left those two other commented changes as they are. The sealed case class or some other exhaustive compiler checked enum should stay, but text grouping can go away, if it's complicating code base too much. But for overal performace reasons, it should be better if it stays. |
|
Thanks for merging this! I tried to change the case class to object, but then exhaustive checks did not work. Maybe I did something funky, but I think it must be case class? |
Hi, this PR implements groubBy option for register report. With groupBy it is possible to control Abandon's register reports grouping time span. This is handy if there is need to generate register report over month or week but with day resolution.
It might be that abandon's current register report is not exactly "register" report, but it is useful as it is and this groupBy adds handy feature to that report. For balance report and xml exports futher desing is needed before those are implemented, so that e.g. formatting is meaningful.