Skip to content
This repository was archived by the owner on Jan 29, 2026. It is now read-only.

feat: rename id to recommendationId#65

Merged
mmiermans merged 1 commit intomainfrom
MC-204-recommendation-id-rename
Oct 17, 2023
Merged

feat: rename id to recommendationId#65
mmiermans merged 1 commit intomainfrom
MC-204-recommendation-id-rename

Conversation

@mmiermans
Copy link
Copy Markdown
Contributor

@mmiermans mmiermans commented Oct 16, 2023

Goal

Be more consistent with naming between Glean and the API.

I'd love feedback/perspectives on:

Is this naming better? The plan is to also apply the renaming to the MoSo content feed API.

Deployment steps

  • Sync with Chutten to check that this attribute is not yet used in Nightly. It doesn't look like the Firefox change is reviewed yet.

References

JIRA ticket:

Glean Bugzilla bug:

Slack thread where naming was discussed:

Firefox change:

@mmiermans mmiermans requested a review from Herraj October 16, 2023 16:08
@mmiermans mmiermans requested review from a team as code owners October 16, 2023 16:08
@mmiermans mmiermans requested review from ScottDowne, efixler and n0coast and removed request for n0coast October 16, 2023 16:08
@mmiermans
Copy link
Copy Markdown
Contributor Author

mmiermans commented Oct 16, 2023

Seems this change is timely, given a comment from @ScottDowne yesterday:

It is my understanding that we can at some point migrate away from id: item.tileId,, and just use recommendation_id: item.id,? Is that correct? Otherwise, I think it's kinda confusing to have all these different ids with names that are not super descriptive.

Copy link
Copy Markdown
Contributor

@Herraj Herraj left a comment

Choose a reason for hiding this comment

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

🆔

Copy link
Copy Markdown
Contributor

@ScottDowne ScottDowne left a comment

Choose a reason for hiding this comment

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

Looks good to me. Do we want to make sure chutten has had a chance to respond before merging?

@mmiermans mmiermans merged commit 598d2d8 into main Oct 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants