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

Reverting: Adding height and width to elements that can contain embedded content #193

Merged
merged 2 commits into from
Feb 1, 2021

Conversation

frj
Copy link
Contributor

@frj frj commented Jan 29, 2021

This reverts the changes to add height and width to various element types.

#191

This change turned out not to be useful so its being removed to avoid unused clutter.

Copy link
Member

@JustinPinner JustinPinner left a comment

Choose a reason for hiding this comment

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

👍 (if these definitely didn't end up in the published models)

12: optional i32 height
13: optional i32 width
14: optional string sourceDomain
12: optional string sourceDomain
Copy link
Contributor

Choose a reason for hiding this comment

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

In Thrift are you allowed to recycle a label like this?
Honestly don't know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot tony: https://diwakergupta.github.io/thrift-missing-guide/

Don’t change the numeric tags for any existing fields.

@frj
Copy link
Contributor Author

frj commented Jan 29, 2021

👍 (if these definitely didn't end up in the published models)

The content-api-model libs were published but the content-api-scala-client wasnt published with those models libs.

Lets discuss on monday.

@frj
Copy link
Contributor Author

frj commented Feb 1, 2021

Apologies, I mixed up the order I created the new fields.

Width and height did actually make it out into the wild.

Im slightly nervous about making this change now, there does seem to be some danger around removing fields in this way.

As Tony pointed out, its important that the field ids are not reused: https://diwakergupta.github.io/thrift-missing-guide/

Non-required fields can be removed, as long as the tag number is not used again in your updated message type (it may be better to rename the field instead, perhaps adding the prefix "OBSOLETE_", so that future users of your .thrift can’t accidentally reuse the number).

The intention was to 'tidy up' the schema, and leaving around fields with "OBSOLETE_" or commented out fields doesn't feel very tidy.

Its entirely possible these fields will be useful again in the future.

Maybe the best option is just to remove the code that generates these fields out of CAPI, but leave these fields in the schema.

Does anyone have any opinions on this?

Its important the field ids are not reused in the future.
Copy link
Contributor

@tonytw1 tonytw1 left a comment

Choose a reason for hiding this comment

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

This looks like the correct way to deal with removal of optional fields.
Retire the label and never reuse it

@frj frj changed the title Adding height and width to elements that can contain embedded content Reverting: Adding height and width to elements that can contain embedded content Feb 1, 2021
@frj frj merged commit 2b9b097 into master Feb 1, 2021
@frj frj deleted the francis/removing-height-and-width-fields branch February 1, 2021 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants