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

Issue 85: Use time.Time for encoding LogMessage.Time instead of uint64 #89

Merged
merged 2 commits into from
May 17, 2023

Conversation

ParasRaba155
Copy link
Contributor

@ParasRaba155 ParasRaba155 commented May 16, 2023

  • PR for Use time.Time for LogMessage.Time #85

  • CHANGED the LogMeesage.Time encoding from uint64 to time.Time and And adjusted the testcases to check if time is not zero

  • Removed the redudant error check in reciver.go as linter suggested

…djusted the testcases to check if time is not zero

- Removed the redudant error check in reciver.go as golint suggested
@coveralls
Copy link
Collaborator

coveralls commented May 16, 2023

Coverage Status

Coverage: 94.04% (+0.2%) from 93.844% when pulling 7deb406 on ParasRaba155:change_logmsg_time_encoding into 8009536 on roc-streaming:main.

@gavv
Copy link
Member

gavv commented May 16, 2023

Hi, thanks for PR! LGTM.

Removed the redudant error check in reciver.go as linter suggested

AFAIK, we don't have such linter enabled, and the code base doesn't follow the suggested code style, so could you please roll back that part?

I personally prefer the existing code style, too - every error checking block looks the same no matter if it's the last one or not. With this approach, you can easily reorder blocks or add a new one to the end. With the suggested approach, the last block always requires special handling, which is annoying.

@gavv gavv added the needs revision Pull request should be revised by its author label May 16, 2023
@gavv gavv linked an issue May 16, 2023 that may be closed by this pull request
@ParasRaba155
Copy link
Contributor Author

AFAIK, we don't have such linter enabled,

as for the linter, that was the golangci-lint suggestion on make lint command, but that might be due to different version of golangci-lint but I will roll back that and also, can you specify me the golangcli-lint version that is used in the project so that I can verify mine.

@gavv gavv merged commit 071de33 into roc-streaming:main May 17, 2023
@gavv
Copy link
Member

gavv commented May 17, 2023

Oh, I see, thanks.

CI uses 1.51.2
https://github.com/roc-streaming/roc-go/blob/main/.github/workflows/build.yaml#L109

What version do you use?

If you're using a later one, I guess we'll need to adjust the linter config and bump it on CI, too. If you're using an older one, you can just update yours.

@ParasRaba155
Copy link
Contributor Author

CI uses 1.51.2 https://github.com/roc-streaming/roc-go/blob/main/.github/workflows/build.yaml#L109

Mine is 1.52.2 but it is built with go1.20.2, as I have that version of go installed locally and I forgot to install linter from the go1.13

@ParasRaba155 ParasRaba155 deleted the change_logmsg_time_encoding branch May 17, 2023 10:57
@gavv gavv removed the needs revision Pull request should be revised by its author label May 20, 2023
@gavv
Copy link
Member

gavv commented May 20, 2023

A fix for linter was merged in #90.

BTW, welcome to matrix chat of the project! https://roc-streaming.org/toolkit/docs/about_project/contacts.html#matrix-chats

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.

Use time.Time for LogMessage.Time
3 participants