8000 Document RBS block syntax by sambostock · Pull Request #8828 · sorbet/sorbet · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Document RBS block syntax #8828

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

Merged
merged 1 commit into from
May 6, 2025
Merged

Document RBS block syntax #8828

merged 1 commit into from
May 6, 2025

Conversation

sambostock
Copy link
Contributor
@sambostock sambostock commented May 5, 2025

This adds entries to the RBS "quick reference" table for (mandatory) and optional blocks.

Motivation

While sig style signatures use the same syntax to describe blocks as Procs, RBS syntax doesn't.

Test plan

This is a documentation only change.

@sambostock sambostock force-pushed the patch-2 branch 3 times, most recently from 1115625 to b0dc475 Compare May 5, 2025 19:39
Comment on lines 245 to 246
| [Block type] | `{ (Foo) -> Bar }` | [`T.proc.params(arg: Foo).returns(Bar)`](procs.md) |
| [Optional Block type] | `?{ (Foo) -> Bar }` | [`T.nilable(T.proc.params(arg: Foo).returns(Bar))`](procs.md#optional-blocks) |
Copy link
Contributor Author
@sambostock sambostock May 5, 2025

Choose a reason for hiding this comment

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

I originally had these as a single row with <br>, but that didn't look great.

This looks better, although the optional block line ends up split over two lines due to the way the table gets formatted. I might try opening a different PR that switches the last column to use actual ```ruby code blocks, and spread the type_parameters example over multiple lines, which I suspect would fix that and read better anyway (on top of giving us formatting for the Sorbet code).

image

Copy link
Contributor Author
@sambostock sambostock May 5, 2025

Choose a reason for hiding this comment

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

I've opened #8831 to change the table/snippet formatting. If we combine the changes from this PR and #8831, we get

This PR only (#8828) Also with changes from #8831
image image

which IMO looks better than without #8831's change.

Copy link
Contributor Author
8000

Choose a reason for hiding this comment

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

#8831 is being merged, so I'll rebase ahead of it to streamline review.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ fyi. @jez

@sambostock sambostock marked this pull request as ready for review May 5, 2025 22:35
@sambostock sambostock requested a review from a team as a code owner May 5, 2025 22:35
@sambostock sambostock requested review from froydnj and removed request for a team May 5, 2025 22:35
@sambostock sambostock force-pushed the patch-2 branch 2 times, most recently from 36f2b1a to 844b074 Compare May 5, 2025 23:40
@jez jez enabled auto-merge (squash) May 6, 2025 00:03
@sambostock
Copy link
Contributor Author

:linux: linters.sh appears to be failing, but I can't tell why

While `sig` style signatures use the same syntax to describe blocks as
`Proc`s, RBS syntax doesn't.
auto-merge was automatically disabled May 6, 2025 00:17

Head branch was pushed to by a user without write access

@sambostock
Copy link
Contributor Author

Nevermind, it was the reference link indentation. I've pushed a fix.

cc. @jez

@sambostock sambostock requested a review from jez May 6, 2025 00:19
@jez jez enabled auto-merge (squash) May 6, 2025 00:47
@jez jez merged commit e699fa6 into sorbet:master May 6, 2025
14 checks passed
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.

3 participants
0