Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Tweak a couple of recipe fields to be arrays of strings instead of strings #244

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

frederickobrien
Copy link
Contributor

@frederickobrien frederickobrien commented Mar 13, 2024

What does this change?

Fixing a silly mistake with the thrift. Although it's usually a no-no to change the shape of thrift fields in this case we should be in the clear given it's not being used anywhere yet

How to test

How can we measure success?

Have we considered potential risks?

Images

Accessibility

@frederickobrien frederickobrien requested a review from a team as a code owner March 13, 2024 13:24
Comment on lines -25 to -26
17: optional string suitableForDiet
18: optional string cookingMethod
Copy link
Contributor

Choose a reason for hiding this comment

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

These are now backwards-incompatible changes, right? (Assuming you made a release with your previous changes.) Should we deprecate these fields and add new ones, instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having chatted with @fredex42 (who confirmed with @paulmr) we can get away with it in this case as it's not being used anywhere yet

Copy link
Contributor

Choose a reason for hiding this comment

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

I know there shouldn’t be any existing data using these fields with these types, but maybe in the future someone might upgrade to the version we’ve just released that includes the incorrect types?

Copy link
Contributor

Choose a reason for hiding this comment

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

@emdash-ie The thing is, Concierge will never actually generate data with this schema. So, if somebody does accidentally update to this version then they will get a 500 error if there is any (list) data in these fields.

That won't be an issue unless they have explicitly requested schema.org content with the schemaOrg=true parameter.

The difference is that Concierge will never have generated data in this form, so I think it's right to update the library asap before the incorrect version has the chance to spread.

@frederickobrien frederickobrien merged commit 7ec7c8c into main Mar 13, 2024
1 check passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants