search

valebes / rsClock

A simple terminal clock written in Rust.
MIT License
98 stars 6 forks source link

Use `u8::wrapping_*` instead of `*_u8`, add gitignore #15

Closed Kyllingene closed 1 year ago

Kyllingene commented 1 year ago

In your code, you use custom functions inc_u8 and dec_u8 instead of the built-in add_wrapping and sub_wrapping functions. I removed the unnecessary functions in favor of the built-in methods.

There's also no gitignore, which makes contributing annoying (since you have to explicitly ignore the target directory for each commit), so I added a simple .gitignore file that only excludes the target directory.

I also have branches for two 12-hour display modes. One follows issue #9 (branch line-12hr), and one displays the AM/PM alongside the time in block style (branch block-12hr). If either of these seem helpful, let me know and I'll create a PR.

valebes commented 1 year ago

@Kyllingene I really appreciate the contribution. I'll take a look at the diff and the proposals you made right away.

valebes commented 1 year ago

So, for me this PR is absolutely fine. As for the 12-hour display mode, I took a look at both variants. The one with the AM/PM lock is very nice, but maybe for people used to ttyclock would prefer to have the display next to the date (as you already did in the other variant). (Maybe it would be a good idea to put both? What do you think?) In case you are interested in doing another PR on this, you are welcome. Thanks again for your contribution.

valebes commented 1 year ago

I'll wait to release the new release in case you want to do the other PR as well.

Kyllingene commented 1 year ago

No problem! I use rsClock as a "screensaver" a lot, so it's really great that you're active on it.

I went ahead and implemented both 12-hour modes, there should be a PR for that now.