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

Better typespecs #367

Merged
merged 8 commits into from
Sep 14, 2023
Merged

Better typespecs #367

merged 8 commits into from
Sep 14, 2023

Conversation

scohen
Copy link
Collaborator

@scohen scohen commented Sep 14, 2023

This PR has three commits, the first is the biggest, and adds (very) detailed type information to our generated modules. Thanks to this, i found a couple legit bugs in the code.

I also had to nudge the generators slightly in order to get them to generate base types, which were missing from the codebase. Since the missing things were just aliases to base types, stuff would still work.

Our protocol types were rather weak, relying on most of the structs
defining their type as `@type t :: %__MODULE__{}`. We can do _much_
better than that, since we have detailed infomration about the entire
type hierarchy.

This commit does that providing detailed type information for the
created structs and their constructors.

I also found a couple of cases where the generators would miss type
aliases that were just aliases to "base" types, like strings,
integers, etc.
Copy link
Collaborator

@zachallaun zachallaun left a comment

Choose a reason for hiding this comment

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

I think this looks good. My only real suggestion is that I think things would be a bit more obvious if Typespec were only responsible for constructing the bits on the right of ::. See various comments.

apps/proto/lib/lexical/proto/macros/message.ex Outdated Show resolved Hide resolved
apps/proto/lib/lexical/proto/alias.ex Outdated Show resolved Hide resolved
apps/proto/lib/lexical/proto/response.ex Outdated Show resolved Hide resolved
apps/proto/lib/lexical/proto/type.ex Outdated Show resolved Hide resolved
apps/proto/lib/lexical/proto/macros/struct.ex Outdated Show resolved Hide resolved
@scohen
Copy link
Collaborator Author

scohen commented Sep 14, 2023

I think things would be a bit more obvious if Typespec were only

I considerd this, your comment confirms my hunch, I'll change it.

Fixed missed cases for typespecs, fixed unit test that was asserting
the wrong value type.
@scohen scohen merged commit 2ea7be2 into main Sep 14, 2023
6 checks passed
@scohen scohen deleted the better_typespecs branch September 14, 2023 22:42
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