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

[DIA-2458] Review params to GET /messages endpoint #671

Merged
merged 7 commits into from
Aug 1, 2023

Conversation

bohdan-go-wombat
Copy link
Collaborator

No description provided.

@bohdan-go-wombat bohdan-go-wombat self-assigned this Jul 31, 2023
@bohdan-go-wombat bohdan-go-wombat added the 🔎 ready for review This PR is ready for review label Jul 31, 2023
internal data class OperatingSystemInfoParam(
@SerialName("name")
val name: String? = DEFAULT_ANDROID_OS_NAME,
@SerialName("version")
Copy link
Member

Choose a reason for hiding this comment

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

@bohdan-go-wombat we don't need to add the @SerialName notation if the attribute in the data class has the same name as on the final json.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andresilveirah yes, we can exclude this annotation in that case, but then we stumble upon the thing when we have a bunch of params marked with the annotation, while others are not.

I think the code is more structured and "clear" this way, when you explicitly point out the name of the attribute, even if the name is the same with the param of the model inside the native code.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you, if we'd have many fields with different names, that'd be confusing. But I believe that's not the case right? From what I could see, most json fields have identical names as the attributes with a few exceptions.

We might be getting in the realm of "opinion" now, but I believe this:

@Serializable
data class IncludeData(
    val localState: IncludeDataParam? = null,
    val tcData: IncludeDataParam? = null,
    val campaigns: IncludeDataParam? = null,
    val customVendorsResponse: IncludeDataParam? = null,
    val messageMetaData: IncludeDataParam? = null,
    val webConsentPayload: IncludeDataParam? = null,
)

Is clearer than this:

@Serializable
data class IncludeData(
    @SerialName("localState")
    val localState: IncludeDataParam? = null,
    @SerialName("TCData")
    val tcData: IncludeDataParam? = null,
    @SerialName("campaigns")
    val campaigns: IncludeDataParam? = null,
    @SerialName("customVendorsResponse")
    val customVendorsResponse: IncludeDataParam? = null,
    @SerialName("messageMetaData")
    val messageMetaData: IncludeDataParam? = null,
    @SerialName("webConsentPayload")
    val webConsentPayload: IncludeDataParam? = null,
)

And, do we need to assign null if the attribute is optional? Could we replace, for example, this:

val webConsentPayload: IncludeDataParam? = null,

with this:

val webConsentPayload: IncludeDataParam?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. It is definitelly a matter of opinion, and from my point of view this:
@Serializable
data class IncludeData(
    @SerialName("localState")
    val localState: IncludeDataParam? = null,
    @SerialName("TCData")
    val tcData: IncludeDataParam? = null,
    @SerialName("campaigns")
    val campaigns: IncludeDataParam? = null,
    @SerialName("customVendorsResponse")
    val customVendorsResponse: IncludeDataParam? = null,
    @SerialName("messageMetaData")
    val messageMetaData: IncludeDataParam? = null,
    @SerialName("webConsentPayload")
    val webConsentPayload: IncludeDataParam? = null,
)

is more readable and clear than this:

@Serializable
data class IncludeData(
    val localState: IncludeDataParam? = null,
    @SerialName("TCData") val tcData: IncludeDataParam? = null,
    val campaigns: IncludeDataParam? = null,
    val customVendorsResponse: IncludeDataParam? = null,
    val messageMetaData: IncludeDataParam? = null,
    val webConsentPayload: IncludeDataParam? = null,
)

especially, when the IDE highlights what is the parameter of the model itself and what is the name for the serizalization.


  1. Regarding your second question, we need to have default values, which allows us to create an instance of IncludeData without passing null to each parameter we don't need, so it looks like this:
fun generateIncludeDataForMessages(): IncludeData = IncludeData(
    tcData = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
    campaigns = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
    webConsentPayload = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
)

and not like this:

fun generateIncludeDataForMessages(): IncludeData = IncludeData(
    localState = null,
    tcData = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
    campaigns = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
    customVendorsResponse = null,
    messageMetaData = null,
    webConsentPayload = IncludeDataParam(IncludeDataParamType.RECORD_STRING.type),
)

@bohdan-go-wombat bohdan-go-wombat changed the title [DIA-2458] Review message param [v2] [DIA-2458] Review params to GET /messages endpoint Aug 1, 2023
@bohdan-go-wombat bohdan-go-wombat marked this pull request as ready for review August 1, 2023 10:40
@bohdan-go-wombat bohdan-go-wombat added the ✔️ ready for release This PR is ready for release, make sure all the checks are passed and merge label Aug 1, 2023
@bohdan-go-wombat bohdan-go-wombat merged commit 6880c75 into develop Aug 1, 2023
4 checks passed
@bohdan-go-wombat bohdan-go-wombat deleted the DIA-2458-review-message-param-v2 branch August 1, 2023 13:32
@bohdan-go-wombat bohdan-go-wombat removed the 🔎 ready for review This PR is ready for review label Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✔️ ready for release This PR is ready for release, make sure all the checks are passed and merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants