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 diagnostic message when shape fields are missing #8762

Merged
merged 3 commits into from Nov 26, 2022

Conversation

muglug
Copy link
Collaborator

@muglug muglug commented Nov 25, 2022

This is a small DX improvement so that developers who run into issues with Psalm 5 sealed array shapes have a bit more information about why the shapes were incompatible.

Before:

Argument 1 of a expects array{test: string}, but array{somethingElse: 'test', test: 'str'} provided

After:

Argument 1 of a expects array{test: string}, but array{somethingElse: 'test', test: 'str'} with additional array shape fields (somethingElse) was provided

@muglug muglug added the release:internal The PR will be included in 'Internal changes' section of the release notes label Nov 25, 2022
@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

How does that look with nested shapes? E.g. container type array{data:array{id:int}} with input type array{data:array{di:int}}.

@muglug
Copy link
Collaborator Author

muglug commented Nov 25, 2022

It should give the missing shapes I think, since they're just propagated up the TypeComparisonResult

Edit: they're not, yet

@weirdan
Copy link
Collaborator

weirdan commented Nov 25, 2022

What I was asking was whether Psalm would help me identify the specific nested key that was missing. By, e.g., printing the full path like data.some.deeply.nested.array.shape.path.id for

array{data:array{some:array{deeply:array{nested:array{shape:array{path:array{id:int}}}}}}}
vs 
array{data:array{some:array{deeply:array{nested:array{shape:array{path:array{di:int}}}}}}}

@orklah
Copy link
Collaborator

orklah commented Nov 25, 2022

Thanks for this! It also unlocks the possibilty of a specific psalter handling, when missing_shape_fields is not empty, then adding fallback_params to the keyed array should fix the issue

@muglug
Copy link
Collaborator Author

muglug commented Nov 26, 2022

@weirdan it doesn't show the nesting (but no reason it couldn't)

@orklah to unlock Psalter handling would require checking the positions of the types. That would require us to start adding more positional information to the Atomic.

It would be a non-trivial task, and would mean we'd probably also want to break this out into a separate issue (which we could do anyway to appease some folks)

@muglug muglug merged commit 1819a2d into vimeo:master Nov 26, 2022
@muglug muglug deleted the muglug-add-shape-field-diagnostic branch November 26, 2022 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants