10000 print more info when created record collides with others by RedemptionC · Pull Request #174 · dominikbraun/timetrace · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

print more info when created record collides with others #174

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 11 commits into from
Sep 12, 2021
Merged

print more info when created record collides with others #174

merged 11 commits into from
Sep 12, 2021

Conversation

RedemptionC
Copy link
Contributor
@RedemptionC RedemptionC commented Aug 28, 2021 •
edited
Loading

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 RedemptionC marked this pull request as draft August 28, 2021 02:55
@RedemptionC RedemptionC marked this pull request as ready for review August 28, 2021 03:08
@RedemptionC
Copy link
Contributor Author

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

@dominikbraun dominikbraun self-requested a review August 28, 2021 07:27
dominikbraun
dominikbraun reviewed Aug 28, 2021
View reviewed changes
Copy link
Owner
@dominikbraun dominikbraun left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author
RedemptionC commented Aug 28, 2021 •
edited
Loading

@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
Copy link
Owner

@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
Copy link
Contributor Author

@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-0
8000
3 | 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
Copy link
Contributor Author

@dominikbraun
hello,what's your opinion?

@dominikbraun
Copy link
Owner

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

dominikbraun
dominikbraun reviewed Sep 3, 2021
View reviewed changes
Copy link
Owner
@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

core/timetrace.go Outdated
@@ -273,6 +275,37 @@ func (t *Timetrace) latestNonEmptyDir(dirs []string) (string, error) {
return "", ErrAllDirectoriesEmpty
}

func printCollides(t *Timetrace, records []*Record) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may call this function printCollisions.

@RedemptionC
refactor: change function name:
65590a3 
printCollides to printCollisions
@RedemptionC
Copy link
Contributor Author

@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
Copy link
Owner

@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
Copy link
Contributor Author

@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
Copy link
Owner

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 dominikbraun self-requested a review September 4, 2021 18:21
dominikbraun
dominikbraun approved these changes Sep 4, 2021
View reviewed changes
Copy link
Owner
@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@RedemptionC
Copy link
Contributor Author

@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 added this to the timetrace v0.14.0 milestone Sep 12, 2021
@dominikbraun
Copy link
Owner

@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 dominikbraun self-requested a review September 12, 2021 20:10
dominikbraun
dominikbraun approved these changes Sep 12, 2021
View reviewed changes
Copy link
Owner
@dominikbraun dominikbraun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine, thanks for implementing this change!

@dominikbraun dominikbraun merged commit a4dfe6e into dominikbraun:main Sep 12, 2021
@RedemptionC
Copy link
Contributor Author
RedemptionC commented Sep 13, 2021 •
edited
Loading

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikbraun dominikbraun dominikbraun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add more information to colliding records
2 participants
@RedemptionC @dominikbraun
0