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

Add ID() to the Block interface #1359

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

samstarling
Copy link

At incident.io, we often iterate over the contents of Slack views, and often want to add the block ID to various bits of observability (logs, traces). Doing this is quite difficult, because the Block interface doesn't allow easy access to the ID — we have to cast each possible kind of block to pull this out.

This PR introduces an ID() function to the Block interface, making this kind of thing much easier.

I added some tests along the way, and also noticed that CallBlock doesn't allow the ID to be set. This doesn't affect us, but you might wish to fix that.

Copy link
Contributor

@calebmckay calebmckay left a comment

Choose a reason for hiding this comment

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

Fixes #1358.

This definitely seems like a good feature to have when parsing a BlockSet from a JSON payload. I'm not sure how often we'd want to set a BlockID manually from the New*Block functions, but I guess the main problem is that it's not currently consistent across block types.

@samstarling
Copy link
Author

samstarling commented Dec 10, 2024

@calebmckay Nice, thanks! I hadn't seen that issue. Can I ask what the path to getting this reviewed/approved is? Just want to make sure I've not missed anything. But yes, you're right — it's useful to (for example) get a payload from Slack and be able to inspect the block IDs without having to do a bunch of type conversion.

@calebmckay
Copy link
Contributor

It looks like @lorenzoaiello is the primary active maintainer right now, so they'll need to review and merge.

@samstarling
Copy link
Author

Hey @parsley42, I was wondering, are you the current maintainer here?

@parsley42
Copy link
Member

No, you want @lorenzoaiello

@nlopes
Copy link
Collaborator

nlopes commented Jan 28, 2025

Although technically this can be a breaking change, I think people should be relying on what we provide so am going to consider this as part of a non-breaking change batch.

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.

4 participants