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

update for jms serializer v2/v3 #3

Merged
merged 3 commits into from Oct 9, 2019

Conversation

@Tobion
Copy link
Member

Tobion commented Oct 9, 2019

No description provided.

@dbu
dbu approved these changes Oct 9, 2019
Copy link
Member

dbu left a comment

cool!

only point i have is about the class name alias. i liked it to be explicit througout the code on what is jms and what is liip serializer context.

use JMS\Serializer\DeserializationContext as JMSDeserializationContext;
use JMS\Serializer\SerializationContext as JMSSerializationContext;
use JMS\Serializer\DeserializationContext;
use JMS\Serializer\SerializationContext;

This comment has been minimized.

Copy link
@dbu

dbu Oct 9, 2019

Member

I did this on purpose to be clear that this is about jms and not about the liip serializer. but if you are sure its better like this i don't mind too much.

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 9, 2019

Author Member

It was not consistent. Why have the Jms alias for the context, but

use JMS\Serializer\SerializerInterface;
use Liip\Serializer\SerializerInterface as LiipSerializer;

was aliased the other way round.

This comment has been minimized.

Copy link
@dbu

dbu Oct 9, 2019

Member

oh, indeed. so now everythink liip serializer is aliased and jms is the "default"? makes sense for me in the adapter.

use JMS\Serializer\DeserializationContext as JMSDeserializationContext;
use JMS\Serializer\SerializationContext as JMSSerializationContext;
use JMS\Serializer\DeserializationContext;
use JMS\Serializer\SerializationContext;
use JMS\Serializer\SerializerInterface;
use Liip\Serializer\Context;

This comment has been minimized.

Copy link
@dbu

dbu Oct 9, 2019

Member

should we alias this to LiipContext to disambiguate when reading the code?

This comment has been minimized.

Copy link
@Tobion

Tobion Oct 9, 2019

Author Member

Don't think it's a good idea to alias both because as soon as the name conflict is not there anymore, you need to change both aliases increasing the diff.

@Tobion Tobion force-pushed the jms-v3 branch from 7a7ebdc to b05a92b Oct 9, 2019
@Tobion Tobion changed the title WIP: update for jms serializer v2/v3 update for jms serializer v2/v3 Oct 9, 2019
@Tobion Tobion merged commit cf87de9 into master Oct 9, 2019
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Tobion Tobion deleted the jms-v3 branch Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.