Skip to content

FIX GRAMMATICAL ERROR#1

Open
aidenwallis wants to merge 3 commits into
pajbot:masterfrom
aidenwallis:patch-1
Open

FIX GRAMMATICAL ERROR#1
aidenwallis wants to merge 3 commits into
pajbot:masterfrom
aidenwallis:patch-1

Conversation

@aidenwallis

Copy link
Copy Markdown

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

@pajlada pajlada left a comment

Copy link
Copy Markdown
Member

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

fourtf commented Aug 30, 2021

Copy link
Copy Markdown

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

@gempir

gempir commented Aug 30, 2021

Copy link
Copy Markdown

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

@pajlada

pajlada commented Aug 30, 2021

Copy link
Copy Markdown
Member

@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
Copy Markdown
Author

eh

@aidenwallis

Copy link
Copy Markdown
Author

hi :) randomly chose to come back

@aidenwallis aidenwallis reopened this Oct 2, 2021
@gempir

gempir commented Oct 2, 2021

Copy link
Copy Markdown

🎃

@aidenwallis aidenwallis requested a review from pajlada October 2, 2021 21:09
@RingoMar

RingoMar commented Oct 2, 2021

Copy link
Copy Markdown

🐛

@pajlada pajlada left a comment

Copy link
Copy Markdown
Member

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.

5 participants