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

FIX GRAMMATICAL ERROR #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

aidenwallis
Copy link

it should be 0 seconds not 0 second

i cannot believe you did this ❗ ❗

it should be `0 seconds` not `0 second`

i cannot believe you did this ❗ ❗
pajlada
pajlada previously approved these changes Aug 30, 2021
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Very cool! Thanks for your contribution!

@fourtf
Copy link

fourtf commented Aug 30, 2021

what if there's a negative value somehow? then it would be -1 seconds? that seems odd to me

@gempir
Copy link

gempir commented Aug 30, 2021

You didn't even add a test case to prevent this issue in the future

@pajlada
Copy link
Member

pajlada commented Aug 30, 2021

@aidenwallis Actually I'll retract my approval for now - could you provide a test case in which this case errors out? This should be pretty well-tested already to never print zero values

https://github.com/pajbot/utils/blob/master/time_test.go#L59-L77

@aidenwallis
Copy link
Author

eh

@aidenwallis
Copy link
Author

hi :) randomly chose to come back

@aidenwallis aidenwallis reopened this Oct 2, 2021
@gempir
Copy link

gempir commented Oct 2, 2021

🎃

@RingoMar
Copy link

RingoMar commented Oct 2, 2021

🐛

Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

The test does not show a case where the change is needed

Reverting the change to time.go#64 still passes all tests

image

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.

None yet

5 participants