8000 Add basic format option by sfrique · Pull Request #114 · dominikbraun/timetrace · GitHub
[go: up one dir, main page]

Skip to content

Add basic format option - #114

Merged
dominikbraun merged 2 commits into
dominikbraun:mainfrom
sfrique:issue-109/add-format
Jun 12, 2021
Merged

dominikbraun merged 2 commits into
dominikbraun:mainfrom
sfrique:issue-109/add-format

Conversation

@sfrique
@sfrique sfrique commented Jun 11, 2021
Copy link
Copy Markdown
Contributor

Initial very basic implementation for #109

I didn't write any test and actually used fmt.Printf should I create an out.Printf or something like this?

Please any suggestions are welcome, I am not really a golang dev =P

To have better options to replace and format, we would need to add more information to the status command.

Comment thread cli/status.go Resolved
@dominikbraun dominikbraun left a comment
Copy link
Copy Markdown
Owner

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! I'd merely suggest to directly return in case a format flag has been provided, to better indicate that this flag is a point of return in the program.

Co-authored-by: Dominik Braun <mail@dominikbraun.io>
@dominikbraun
dominikbraun self-requested a review June 11, 2021 19:31
@dominikbraun
dominikbraun merged commit 3fd9ae3 into dominikbraun:main Jun 12, 2021
@dominikbraun
Copy link
Copy Markdown
Owner

FYI: I briefly renamed two of the formatting variables, see #123.

@sfrique
sfrique commented Jun 12, 2021
Copy link
Copy Markdown
Contributor Author

FYI: I briefly renamed two of the formatting variables, see #123.

Cool, I reviewed the PR.
One thing to note is that I went for the variable at hand and the Formatter func names.
You might want to update the FormatBreakTime and FormatTodayTime as well.

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.

2 participants

0