Skip to content

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

Merged
gavv merged 2 commits into
roc-streaming:mainfrom
ParasRaba155:change_logmsg_time_encoding
May 17, 2023
Merged

Issue 85: Use time.Time for encoding LogMessage.Time instead of uint64#89
gavv merged 2 commits into
roc-streaming:mainfrom
ParasRaba155:change_logmsg_time_encoding

Conversation

@ParasRaba155

@ParasRaba155 ParasRaba155 commented May 16, 2023

Copy link
Copy Markdown
Contributor
  • 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

coveralls commented May 16, 2023

Copy link
Copy Markdown
Collaborator

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

gavv commented May 16, 2023

Copy link
Copy Markdown
Member

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 status: 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
Copy Markdown
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

gavv commented May 17, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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 status: needs revision Pull request should be revised by its author label May 20, 2023
@gavv

gavv commented May 20, 2023

Copy link
Copy Markdown
Member

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