Skip to content

Conversation

@Succo
Copy link

@Succo Succo commented Feb 10, 2023

According to https://pkg.go.dev/time#NewTicker
It is a good practice to Stop a ticker when it goes unused to release the ressources a goroutine in this case)

@pulak-opti
Copy link
Contributor

Hi @Succo, thanks for opening this PR!

Can you please update your branch with master?

@Succo
Copy link
Author

Succo commented Apr 18, 2023

Hey @pulak-opti

No problem, it should be good

@Succo
Copy link
Author

Succo commented Apr 18, 2023

Mmm I'm not sure the failed test are related to my review

"Error: Input required and not supplied: token"

Looks like a config issue

Copy link
Contributor

@pulak-opti pulak-opti left a comment

Choose a reason for hiding this comment

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

LGTM!
Mentioning @yasirfolio3 for a final check!

Copy link
Contributor

@yasirfolio3 yasirfolio3 left a comment

Choose a reason for hiding this comment

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

LGTM, can you please update the headers with the current year 2023 in all the files.

@Succo
Copy link
Author

Succo commented Apr 20, 2023

Yes

@pulak-opti
Copy link
Contributor

Hi @Succo
Please sign the CLA as per the contributing guideline.
https://github.com/optimizely/go-sdk/blob/master/CONTRIBUTING.md

@Succo
Copy link
Author

Succo commented May 12, 2023

Should be good.
Let me know if you didn't get it

@pulak-opti
Copy link
Contributor

Hi @Succo
Please make sure the user associate with the commits is same as @Succo. I see it is showing up as "Fabrice Vaillant".

@Succo
Copy link
Author

Succo commented May 12, 2023

I changed the user and force pushed the branch with the new author

@pulak-opti
Copy link
Contributor

The fix is merged here #375.

Thank you for your contribution

@pulak-opti pulak-opti closed this May 15, 2023
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.

3 participants