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

roc-streaming / roc-go

Golang bindings for Roc Toolkit.

https://roc-streaming.org

MIT License

22 stars 10 forks source link

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

Closed ParasRaba155 closed 1 year ago

ParasRaba155 commented 1 year ago

PR for #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

coveralls commented 1 year ago

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

gavv commented 1 year ago

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.

ParasRaba155 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

gavv commented 1 year ago

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