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

Make external vector type non-opaque #4

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

MegaRedHand
Copy link
Contributor

First of all, thanks for such a wonderful library! Now onto the PR changes:

This PR makes the struct a non-opaque type, and any change to the %Aja.Vector{__vector__: ...} a breaking change. However, this allows matching against the type in guards, without dialyzer spitting "Call does not have expected opaque term ..." errors, while maintaining the opaqueness for the inner raw type.

This goes in line with this change on MapSet. For more context on the error, see this forum post.

This allows matching against the type in guards, without dialyzer spitting "Call does not have expected opaque term ..." errors.

This goes in line with [this change on `MapSet`](https://github.com/elixir-lang/elixir/pull/11917/files). For more context on the error, see [this forum post](https://elixirforum.com/t/dialyzer-complaint-about-mapset-member-not-getting-proper-type-as-argument-possible-specs-bug-in-mapset/20780/17).
@sabiwara
Copy link
Owner

sabiwara commented Jan 3, 2024

Hi @MegaRedHand, thanks a lot for the pull request!

You're right, it seems I did fix it for OrdMap a long time ago.

I used a different strategy back then, probably to enable pattern-matching using the ord/1 macro and to avoid documenting the internal keys. Will play a bit with dialyzer and try to align these two approaches.

@sabiwara
Copy link
Owner

sabiwara commented Jan 3, 2024

Will merge this and rework it to use a private type so that the contribution is kept. Thank you!

@sabiwara sabiwara merged commit e5a719e into sabiwara:main Jan 3, 2024
6 checks passed
@MegaRedHand MegaRedHand deleted the patch-1 branch January 3, 2024 02:46
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.

2 participants