8000 [Issue #36] Remove hard-coded "15:04" strings by aaronsheah · Pull Request #40 · dominikbraun/timetrace · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Issue #36] Remove hard-coded "15:04" strings #40

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 3 commits into from
May 19, 2021
Merged

[Issue #36] Remove hard-coded "15:04" strings #40

merged 3 commits into from
May 19, 2021

Conversation

aaronsheah
Copy link
Contributor
@aaronsheah aaronsheah commented May 18, 2021 •
edited
Loading

Fixes #36

Changes:

  • Replaced hardcoded 15:04 (and 12 hour version 3:04PM) format string with a constant

Might be best to refactor this logic into a formatter component that is generated by the Config(), @dominikbraun what do you think?

@aaronsheah aaronsheah marked this pull request as ready for review May 18, 2021 14:11
@dominikbraun
Copy link
Owner

Might be best to refactor this logic into a formatter component that is generated by the Config(), @dominikbraun what do you think?

The original idea was that every package knows the default configuration values it needs, and modifies those values based on the global config. But making the config responsible for formatting those values makes sense as well, so I'd say you could give it a try 😄

@aaronsheah
Copy link
Contributor Author

@dominikbraun that's a good point, with that in mind, maybe this new component im thinking of may be returning the date time format instead of the actual date time string itself (or maybe returns the formatter itself) will give it a try

@aaronsheah
Copy link
Contributor Author
aaronsheah commented May 18, 2021 •
edited
Loading

@dominikbraun what do you think about the changes and the introduction of Formatter struct? we could also potentially move

timetrace/core/timetrace.go

Lines 195 to 207 in f3fc51a

func formatDuration(duration time.Duration) string {
hours := int64(duration.Hours()) % 60
minutes := int64(duration.Minutes()) % 60
secods := int64(duration.Seconds()) % 60
// as by convention if the duarion is < then 60 secods
// return "0h 0min Xsec"
if hours == 0 && minutes == 0 {
return fmt.Sprintf("0h 0min %dsec", secods)
}
return fmt.Sprintf("%dh %dmin", hours, minutes)
}
into the Formatter struct too and consolidate all formatting.

@dominikbraun
Copy link
Owner

Yes, the Formatter struct seems to make sense, and it would also be desirable to bind the formatDuration function to that struct. 👍

@aaronsheah aaronsheah force-pushed the 36-remove-hardcoded-strings branch from f3fc51a to 7694f6f Compare May 18, 2021 22:05
@aaronsheah
Copy link
Contributor Author
aaronsheah commented May 18, 2021 •
edited
Loading

@dominikbraun I will move the formatDuration in a separate PR, will likely move Report into its own file as part of that refactor. Feel free to review and merge if it looks good :)

dominikbraun
dominikbraun approved these changes May 19, 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.

LGTM, thanks for your improvements so far!

@dominikbraun
Copy link
Owner
8000

@aaronsheah FYI #34 also contains formatting logic, we'll probably have to refactor this as well.

@aaronsheah
Copy link
Contributor Author
aaronsheah commented May 19, 2021 •
edited
Loading

@dominikbraun I'll fix the conflict of this PR then I'll help with #34

@aaronsheah aaronsheah force-pushed the 36-remove-hardcoded-strings branch from 7694f6f to 57a86d9 Compare May 19, 2021 11:58
@dominikbraun
Copy link
Owner

Thanks! 👍

@dominikbraun dominikbraun self-requested a review May 19, 2021 12:06
@aaronsheah
Copy link
Contributor Author

@dominikbraun feel free to merge this PR, for #34 I have this proposal PR aaronsheah#1 that is built on top of this PR to achieve the same change. I can create that new PR once this PR is merged

@dominikbraun dominikbraun merged commit 488588e into dominikbraun:main May 19, 2021
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.

Remove hard-coded "15:04" strings
2 participants
@aaronsheah @dominikbraun
0