print more info when created record collides with others

[RedemptionC]

resolve #173
first PR in this repo
I simply changed/added some print statements
any feedback will be appreciated

now the output is

❯ go run . create record open-summer today 8:25 10:30
❗ collides with these records :
💡 open-summer 2021-08-28 08:21:00 +0800 CST-2021-08-28 09:23:00 +0800 CST
💡 open-summer 2021-08-28 09:30:00 +0800 CST-2021-08-28 10:20:00 +0800 CST
⚠️  start and end of the record should not overlap with others

[RedemptionC]

@dominikbraun
the current implementation prints all colliding records,is that OK?
or should we just print the time duration occupied by those records?

[dominikbraun]

This looks pretty good to me! Maybe we can format the record start time using the Formatter? In that case, you would have to return the colliding record from collides as a second return value and format its time using the t.Formatter()

Something like t.Formatter().PrettyDateString() + " " + t.Formatter().TimeString() should work (you also may create a new method for this).

[RedemptionC]

@dominikbraun
that's good
But,here are some decisions to be made:

1. do we print all info of a record? start,end,Project
2. for those records whose end is nil(maybe they are recording),should we print its "end",calculate it by add time elapsed to Start?
3. do we need to use some table-like output? like when you run timetrace list ...
   because if we just use out.* , the output sometimes will get messy,like:

❗ collides with these records :
💡  dashboard@open-summer Saturday, 28. August 2021 [16:21,16:21]
💡  open-summer Saturday, 28. August 2021 [09:30,10:20]
💡  open-summer Saturday, 28. August 2021 [08:21,09:23]
⚠️  start and end of the record should not overlap with others

[dominikbraun]

@RedemptionC For 1. and 2.: Yes, I think that makes sense. Regarding 3., a table output might be a good fit indeed - you're free to test and play around a bit and we'll see what you prefer.

[RedemptionC]

@dominikbraun
Hi,I used the table format

❯ go run . create record open-summer today 8:25 20:39
❗ collides with these records :
+-----+------------------+-----------------------+---------+-------+------------+
|  #  |       KEY        |        PROJECT        |  START  |  END  |  BILLABLE  |
+-----+------------------+-----------------------+---------+-------+------------+
|   1 | 2021-08-28-17-03 | dashboard@open-summer | 17:03   | 20:16 | no         |
|   2 | 2021-08-28-16-21 | dashboard@open-summer | 16:21   | 16:21 | no         |
|   3 | 2021-08-28-09-30 | open-summer           | 09:30   | 10:20 | no         |
|   4 | 2021-08-28-08-21 | open-summer           | 08:21   | 09:23 | no         |
+-----+------------------+-----------------------+---------+-------+------------+
⚠️  start and end of the record should not overlap with others

you could review it again at your convenience

[RedemptionC]

@dominikbraun
hello,what's your opinion?

[dominikbraun]

Hi @RedemptionC, sorry for the delay - I've been busy the last days, but I'm going to review it later today!

[dominikbraun]

Looks great, I like it! I just added a minor suggestion.

[dominikbraun]

We may call this function printCollisions.

[RedemptionC]

@dominikbraun
Thanks for your code review
to get this PR merged,should I change the test file as well?
I mean,now the collides function returns not only a bool value,but also colliding records
should I check them?

[dominikbraun]

@dominikbraun
Thanks for your code review
to get this PR merged,should I change the test file as well?
I mean,now the collides function returns not only a bool value,but also colliding records
should I check them?

You're right, now that collides also returns the colliding records, you could do a quick check whether these are the expected collisions.

Thanks for working on this!

[RedemptionC]

@dominikbraun
Hi,I added some checks in the test file
But I'm not pretty sure whether this is the best practice
Maybe you could review them and give me some advice
Or merge them if they are OK?

[dominikbraun]

Hi @RedemptionC, thanks! The tests look pretty good. In fact, you should always be cautious with reflect, but this is a valid use case. We may check if google/go-cmp would provide any advantage over the current solution with DeepEqual, but for now, this is fine.

[dominikbraun]

Code looks good, I'm going to test everything locally and merge this PR if everything is ok.

[RedemptionC]

@dominikbraun

In fact, you should always be cautious with reflect

Thanks for pointing this out to me
Maybe we can use a simple loop to compare them,like:

func checkConsistent(t *testing.T, expect, result []*Record) {
	sameLen := len(result) == len(expect)
	sameContent := true
           
        
	if sameLen {
		for i := range result {
			if expect[i] != result[i] {
				sameContent = false
			}
		}
	}

	if !(sameLen && sameContent) {
		t.Errorf("should collide with :\n")
		for _, r := range expect {
			t.Errorf("%v\n", r)
		}
		t.Errorf("while collides return :\n")
		for _, r := range result {
			t.Errorf("%v\n", r)
		}
	}

}

[dominikbraun]

@dominikbraun

In fact, you should always be cautious with reflect

Thanks for pointing this out to me
Maybe we can use a simple loop to compare them,like:

func checkConsistent(t *testing.T, expect, result []*Record) {
	sameLen := len(result) == len(expect)
	sameContent := true
           
        
	if sameLen {
		for i := range result {
			if expect[i] != result[i] {
				sameContent = false
			}
		}
	}

	if !(sameLen && sameContent) {
		t.Errorf("should collide with :\n")
		for _, r := range expect {
			t.Errorf("%v\n", r)
		}
		t.Errorf("while collides return :\n")
		for _, r := range result {
			t.Errorf("%v\n", r)
		}
	}

}

This approach is fine as well! 👍

[dominikbraun]

This works fine, thanks for implementing this change!

[RedemptionC]

@dominikbraun
that's great! thanks for reviewing and merging this PR!