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

fix issues that methods does not work with protobuf codec #355

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

anatolysergeev
Copy link
Collaborator

No description provided.

@anatolysergeev anatolysergeev requested review from mijicd and a team as code owners April 19, 2021 18:49
@anatolysergeev anatolysergeev force-pushed the fix-tests-with-protobuf-codec branch from 7941f0e to f486a29 Compare April 19, 2021 19:01
@@ -17,7 +16,7 @@ trait BaseSpec extends DefaultRunnableSpec {

def instantOf(millis: Long): UIO[Instant] = UIO(Instant.now().plusMillis(millis))

implicit val codec: Codec = StringUtf8Codec
implicit val codec: Codec = ProtobufCodec
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you tell me why we need to use Protobufcodec? Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea I am also curious

Copy link
Member

Choose a reason for hiding this comment

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

I presume you're referring to the test usage, not the general motivation :). That said, we don't need to use it in the tests, but @anatolysergeev realized there were some discrepancies with it. To be more precise, the expectation is that the library should work properly given any compatible codec, which wasn't the case here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, for example if we would encode int, long, double with protobuf codec, we would get some extra bytes that would describe a type of our value, but redis don't understand this types, and how to work with that, redis just want to see double (for example). That's why the idea to use ArbitraryType[Long] in some cases was a bad idea. If we use something else except StringUtf8 codec, that could broke contract with redis.
If we use in tests for example protobuf codec, we are gonna see that bound whether we can use ArbitraryType or not. May be we should consider the both codecs in tests, because, i'm afraid that if we would use only protobuf codec than we can miss something with our simple codec.)
Here i just want to show that now all tests are working with protobuf codec. I can change it to String codec now, should i?)

Copy link
Member

Choose a reason for hiding this comment

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

Based on the information from discord regarding the readiness of protobuf codec, I'd say let's postpone on using it in the tests for now. Later on we may fully switch to it, or run them together.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok. I've returned string codec to tests.

@mijicd mijicd merged commit 86a3d9b into zio:master Apr 23, 2021
@anatolysergeev anatolysergeev deleted the fix-tests-with-protobuf-codec branch January 3, 2022 09:28
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.

4 participants