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

[auth] use time.Duration instead of int for intervals #1178

Closed
wants to merge 1 commit into from

Conversation

hezhizhen
Copy link
Contributor

Summary

I think it's clearer to use time.Duration instead of int for intervals because the former contains the quantity and the unit while the latter only contains the quantity.

I wonder if it's possible to convert the Interval field in codeResponse to time.Duration.

How was it tested?

devbox auth whoami

@hezhizhen hezhizhen marked this pull request as ready for review June 19, 2023 13:11
@savil savil requested a review from mikeland73 June 24, 2023 03:03
@hezhizhen
Copy link
Contributor Author

@mikeland73

Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

Duration changes look good but please revert the formatting changes, including extra spaces (unless they were caused by gofmt). This helps keep git blame clean.

We don't enforce a line length, but I usually aim for 80. We generally don't comment on line length in PRs, but in this case they are reverting a conscious decision and not related to the main purpose. That said, happy to reconsider if folks think we should allow longer lines, just think we should do that separately before reformating code.

@hezhizhen
Copy link
Contributor Author

Duration changes look good but please revert the formatting changes, including extra spaces (unless they were caused by gofmt). This helps keep git blame clean.

We don't enforce a line length, but I usually aim for 80. We generally don't comment on line length in PRs, but in this case they are reverting a conscious decision and not related to the main purpose. That said, happy to reconsider if folks think we should allow longer lines, just think we should do that separately before reformating code.

Sorry for the late reply, and apologize first.

I prefer short lines and try to keep them in 80 characters. But I also try to keep it in one line if it only exceeds a little (shorter than 120 characters).

I'll create a new PR with the main purpose only.

@loreto loreto closed this Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants