-
Notifications
You must be signed in to change notification settings - Fork 200
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
Use cardType property instead of theme, since the property is Deprecated #653
base: main
Are you sure you want to change the base?
Use cardType property instead of theme, since the property is Deprecated #653
Conversation
…ted in ProfileCardJson
…ted in ProfileCardJson
Thanks! We need the property for compatibility with older versions of the JSON, as you can see from the failing test. How about adding a comment like "This exists for compatibility" instead of removing it? |
@takahirom Thanks! I added again with explain comment. |
Detekt check failed. Please run |
@@ -11,7 +11,10 @@ internal data class ProfileCardJson( | |||
val occupation: String, | |||
val link: String, | |||
val image: String, | |||
@Deprecated("Use cardType instead", replaceWith = ReplaceWith("cardType")) | |||
@Deprecated( | |||
"Let's use cardType, but keep it for compatibility.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding this. This deprecation message will be shown when developers use this field, so personally the phrase 'keep it' might be a little difficult for developers to understand what they need to do. I recommend adding a separate comment to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, let me update.
Issue
Overview (Required)
ProfileCardJson
, so replace it.Links
Screenshot (Optional if screenshot test is present or unrelated to UI)
Movie (Optional)