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?

[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!

[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.

[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! 👍

[RedemptionC]

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