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

Support for custom serializer #179

Merged
merged 9 commits into from
Feb 22, 2024
Merged

Support for custom serializer #179

merged 9 commits into from
Feb 22, 2024

Conversation

yoobi
Copy link
Contributor

@yoobi yoobi commented Feb 14, 2024

Description

PR is related to issue: #178

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement (Improves existing functionality)

IMPORTANT: By submitting a patch, you agree to allow the project
owners to license your work under the terms of
the Apache License 2.0.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hey there! Thank you for this PR :) Please take a moment to review our community guidelines to make the contribution process easy and effective for everyone involved.

@yoobi yoobi changed the title Issue/178 Support for custom serializer Feb 14, 2024
@yoobi
Copy link
Contributor Author

yoobi commented Feb 14, 2024

There is also a possiblity of non-breaking change for user who use the EmojiInitializer.

Inside the lib AndroidManifest:

<provider
    android:name="androidx.startup.InitializationProvider"
    android:authorities="${applicationId}.androidx-startup"
    android:exported="false"
    tools:node="merge">
    <meta-data
        android:name="io.wax911.emojify.initializer.EmojiInitializer"
        android:value="androidx.startup" />
</provider>

If user is using custom Deserializer
Inside the user AndroidManifest:

<provider
    android:name="androidx.startup.InitializationProvider"
    android:authorities="${applicationId}.androidx-startup"
    android:exported="false"
    tools:node="merge">
    <meta-data
        android:name="com.package.CustomEmojiInitializer"
        android:value="androidx.startup" />
    <meta-data
        android:name="io.wax911.emojify.initializer.EmojiInitializer"
        android:value="androidx.startup"
        tools:node="remove" />
</provider>

Yoobi added 2 commits February 14, 2024 13:27
chores: downgrade version to 1.8.0 as change is no longer breaking + adding readme
@wax911 wax911 linked an issue Feb 19, 2024 that may be closed by this pull request
@wax911
Copy link
Member

wax911 commented Feb 19, 2024

Please run ./gradlew :emojify:spotlessCheck to check for any formatting issues and ./gradlew :emojify:spotlessApply to apply auto fixable

@yoobi
Copy link
Contributor Author

yoobi commented Feb 19, 2024

Sure ! It is done :)

I've noticed that you've tagged this changed as breaking but it is not

@wax911
Copy link
Member

wax911 commented Feb 19, 2024

Sure ! It is done :)

I've noticed that you've tagged this changed as breaking but it is not

Oh, I had seen this #178 (comment) thus the breaking status

@yoobi
Copy link
Contributor Author

yoobi commented Feb 19, 2024

Yes at first it was a breaking change, but I've refacto the code in order to change it as non-breaking. I've explained in this comment

gradle/version.properties Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@wax911
Copy link
Member

wax911 commented Feb 19, 2024

The PR looks solid, I just have one concern though, since the property annotation on the Model class are for kotlinx.serialization unless we make an interface/abstraction for the model and allow users to implement it and provide annotation for deserialization which we can substitute with the interface for usage. Otherwise some of the json property names don't automatically match the model property names. What are your thoughts?

@wax911 wax911 added feature and removed breaking labels Feb 19, 2024
@yoobi
Copy link
Contributor Author

yoobi commented Feb 19, 2024

I have made a couple test with MoshiDeserializer (I have made a CustomDeserializer in the app example so users could experiment with it) and even though there was no Moshi annotation on the Emoji model it did find the right data for supports_fitzpatrick and supports_gender. I could try with Gson or Jackson to make sure if we need to abstract the model or not

@yoobi
Copy link
Contributor Author

yoobi commented Feb 19, 2024

So I've added implementation for Jackson and Gson and they do work fine without the need to expose the Model.
I've made a change in the first emoji to try it out and they did return "true" for supports_fitzpatrick and supports_gender

@wax911
Copy link
Member

wax911 commented Feb 20, 2024

Perfect, this is amazing! Could we just rename AEmojiInitializer to AbstractEmojiInitializer

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Auto approved automated PR

@wax911 wax911 merged commit 4b06463 into AniTrend:develop Feb 22, 2024
9 checks passed
@yoobi
Copy link
Contributor Author

yoobi commented Feb 26, 2024

Hello ! I'm back with bad news :'(
I have tried to implement the new version in my project and it seems what we feared (#179 (comment)) is true and kotlinx-serialization is still needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependecy missing in 1.7.1
2 participants