Skip to content
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.

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

add create record command #118

Merged
merged 4 commits into from
Jun 23, 2021
Merged

Conversation

FelixTheodor
Copy link
Contributor

closes #97

I added the create record command. I also added checks that return an error if the end time is before the start time or the record collides with other records of the same day.

@dominikbraun dominikbraun self-requested a review June 13, 2021 16:06
@dominikbraun dominikbraun added this to the timetrace v0.11.0 milestone Jun 13, 2021
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 good to me! I just requested that tiny cobra.ExactArgs change.

cli/create.go Outdated Show resolved Hide resolved
cli/create.go Show resolved Hide resolved
core/formatter.go Show resolved Hide resolved

// RecordCollides checks if the time of a record collides
// with other records of the same day and returns a bool
func (t *Timetrace) RecordCollides(toCheck Record) (bool, error) {
Copy link
Owner

Choose a reason for hiding this comment

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

Glad that we finally have this function. It should be tested though - you can do it yourself if you want to, otherwise I'll write the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I am not quite sure how to write the test for it. I would be glad to hear your approach - mocking the fs, or just creating a full core in the test? The easiest idea that came to my mind would be refactoring the part of the code that actually checks for collision to another method and just test this one. In this case, no mocking or fiddling with the core would be needed, right?

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, you could just move the collision logic in a function that accepts Record instances and test that function. The loading of the records will be tested separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I just did that and added the tests for it. I think all possible scenarios are tested, but if I missed some it should be easy to add more :)

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.

LGTM, thanks for implementing this! 👍

@dominikbraun dominikbraun merged commit ed69e6d into dominikbraun:main Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement timetrace create record command
2 participants