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

RFC 65: Adding UUIDs into ListBlock #65

Merged
merged 3 commits into from
Oct 11, 2021
Merged

RFC 65: Adding UUIDs into ListBlock #65

merged 3 commits into from
Oct 11, 2021

Conversation

kaedroho
Copy link
Contributor

@kaedroho kaedroho commented Dec 30, 2020

@kaedroho kaedroho added the 1:New label Dec 30, 2020
@kaedroho kaedroho changed the title Adding UUIDs into ListBlock RFC 65: Adding UUIDs into ListBlock Dec 30, 2020
kaedroho added a commit to kaedroho/wagtail that referenced this pull request Jan 26, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
kaedroho added a commit to kaedroho/wagtail that referenced this pull request Jan 26, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
kaedroho added a commit to wagtail/wagtail that referenced this pull request Jan 28, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
gasman pushed a commit to wagtail/wagtail that referenced this pull request Feb 20, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
gasman pushed a commit to wagtail/wagtail that referenced this pull request Mar 5, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
gasman pushed a commit to wagtail/wagtail that referenced this pull request Mar 15, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
gasman pushed a commit to wagtail/wagtail that referenced this pull request Mar 17, 2021
This brings it slightly closer to StreamChild, and we'll need it later
for wagtail/rfcs#65
@kaedroho kaedroho added 2:Accepted and removed 1:New labels Mar 31, 2021
@kaedroho
Copy link
Contributor Author

Assuming accepted given the thumbs up, and no objections when I presented it in the team meeting a couple of months ago

@zerolab
Copy link
Contributor

zerolab commented Apr 7, 2021

Discussed this in the core team meeting and we're all for this. Will require release notes for anyone that works directly with the values

@allcaps
Copy link
Member

allcaps commented Apr 7, 2021

I like this. All blocks have a type and id, so this would make list block consistent. 👏

@allcaps allcaps self-requested a review April 7, 2021 09:07
@Stormheg
Copy link
Member

Stormheg commented Apr 7, 2021

I agree with the approach and don't have any questions or comments.

Thanks @kaedroho and @jacobtoppm 👍

@gasman
Copy link
Contributor

gasman commented Apr 7, 2021

Looks good to me too. I think it's important that we keep the preserve the outward-facing behaviour of ListBlock (i.e. iterating over it gives a list of values without any extra wrapper), but that's been accounted for here. As @zerolab mentions, this does need to be covered in the release notes for anyone with existing project code that works directly the JSON representation and expects to find the old representation.

Just wondering what this should mean for streamfields in API responses? If we're sticking rigorously to the policy of no breaking changes without an API version bump, then we really ought to normalise them to the old representation at the point of output. I'm inclined to say that's overkill in practice though.

@kaedroho
Copy link
Contributor Author

kaedroho commented Apr 9, 2021

Thanks, everyone for your feedback!

Just wondering what this should mean for streamfields in API responses? If we're sticking rigorously to the policy of no breaking changes without an API version bump, then we really ought to normalise them to the old representation at the point of output. I'm inclined to say that's overkill in practice though.

This shouldn't be too difficult to do by overriding get_api_representation. But we probably should make it easy for people to get IDs in their APIs too, maybe a keyword argument on ListBlock / meta to allow people to switch to the new representation in the API?

@kaedroho
Copy link
Contributor Author

Moving to FCP since it looks like all discussions are resolved and people are happy with this change!

@Pomax
Copy link

Pomax commented Sep 22, 2021

It's been a few weeks, hopefully this is still moving ahead? =)

@kaedroho
Copy link
Contributor Author

@Pomax Yep, I think this has had a long enough FCP period now!

@kaedroho kaedroho merged commit 9346256 into main Oct 11, 2021
@kaedroho kaedroho deleted the listblock branch October 11, 2021 10:51
@Pomax
Copy link

Pomax commented Nov 24, 2021

Nice, I missed this landing! Is there a corresponding implementation issue on the wagtail repo that folks can subscribe to, in order to follow the progress there?

@kaedroho
Copy link
Contributor Author

Hey @Pomax I believe this is actually being implemented right now by @gasman and @Tijani-Dia. Should be ready in the next couple of weeks

@Pomax
Copy link

Pomax commented Nov 25, 2021

Awesome! Still, do accepted RFCs in general turn into implementation epics that people can then subscribe to?

@zerolab
Copy link
Contributor

zerolab commented Nov 26, 2021

@Pomax there is no hard rule for that, but usually they will be followed up by a PR (or series of PRs). For this one, see wagtail/wagtail#7737

@Pomax
Copy link

Pomax commented Nov 27, 2021

gotcha, thanks.

@gasman
Copy link
Contributor

gasman commented Nov 30, 2021

PR implementing this RFC: wagtail/wagtail#7741

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants