Report issue#88 #99
Report issue#88 #99
Conversation
… times to be non pointer matching thier zero value by the time.Time.IsZero function. Fixed issue where --end time would not be inclusive
|
@dominikbraun while using the report feature my self I found a bug causing a panic if |
|
Thanks! Is this ready to review already? |
|
Yes it is ready to be reviewed - all of the above mentioned flags are supported (only serialisation to csv is not implemented yet) |
|
@dominikbraun I noticed a merge conflict for this PR after the latest release - I fixed that one PR is ok again. |
cli/report.go
Outdated
| fromTime string | ||
| toTime string |
There was a problem hiding this comment.
As long as we're supporting dates only, I'd probably name this startDate and endDate.
cli/report.go
Outdated
| var fromDate, toDate time.Time | ||
| var formatErr error | ||
|
|
||
| // TODO: find better way to check if flag is default |
There was a problem hiding this comment.
We could use "" as a default here.
cli/report.go
Outdated
| core.FilterNoneNilEndTime, | ||
| core.FilterByTimeRange(fromDate, toDate), |
cli/report.go
Outdated
| Short: "Report allows to view or output tracked records as defined report", | ||
| Run: func(cmd *cobra.Command, args []string) { |
There was a problem hiding this comment.
I'd like to include the report feature in one of the next timetrace releases, but in a sort of beta phase, and officially publish it in its very own release.
| Short: "Report allows to view or output tracked records as defined report", | |
| Run: func(cmd *cobra.Command, args []string) { | |
| Short: "Report allows to view or output tracked records as defined report", | |
| Hidden: true, | |
| Run: func(cmd *cobra.Command, args []string) { |
core/record.go
Outdated
| // apply all filter on record to check if Records should be used | ||
| var useRecord = true | ||
| for _, f := range filter { | ||
| if !f(record) { | ||
| useRecord = false | ||
| break | ||
| } | ||
| } | ||
| // check if eighter filter negates useRecord | ||
| if !useRecord { | ||
| continue | ||
| } |
There was a problem hiding this comment.
You could label the outer loop with outer: and just run
if !f(record) {
continue outer
}This way, we could get rid of the useRecord variable.
There was a problem hiding this comment.
That's why there are Reviews! :D Didn't thought of that - I will implement that.
|
@dominikbraun I have Uni until 6pm I will implement your changes afterwards and update the PR 👍 |
core/formatter.go
Outdated
| year := input.Local().Year() | ||
| day := input.Local().Day() | ||
| weekday := input.Local().Weekday() | ||
| month := input.Local().Month() | ||
|
|
||
| return fmt.Sprintf("%v %d. %v %d", weekday, day, month, year) |
There was a problem hiding this comment.
You can replace this with
| year := input.Local().Year() | |
| day := input.Local().Day() | |
| weekday := input.Local().Weekday() | |
| month := input.Local().Month() | |
| return fmt.Sprintf("%v %d. %v %d", weekday, day, month, year) | |
| return input.Format("Monday, 02. January 2006") |
|
I have incorporated your suggestions and updated the PR |
|
Cool, great to hear! |
# Conflicts: # README.md
This is a first proposal for the
reportcommand.Supported flags:
--billable--project--start/--end--format(json/csv) csv is not yet implemented...--outThe command is designed the way it is discussed in #88.
In the
cli/report.goI commented TODOs. Here I would be happy to get some input as I don't like the way I have implemented the checks if aFlags.StringVarPhas its default value...