Add typespec/1 macro to transform module behaviour
#384
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We detect that `transform_module/0` was defined through a on_definition hook. We then store the module in an attribute. This attribute is later used by the `before_compile` callback to set the typespec, which is set in the form of: ```elixir @t transform_module.t(__MODULE__) ``` Then, transform modules can implement proper typespecs. No changes if `transform_module` is not overriden.
v0idpwn
force-pushed
the
typespec-for-protobuf-transformer
branch
from
475292b to
a6c4647
Compare
|
@whatyouhide @ericmj if you agree on approach here, I'll add a few tests, documentation, etc :) |
v0idpwn
commented
Dec 4, 2024
lib/protobuf/dsl.ex
Outdated
| defp def_t_typespec(%MessageProps{enum?: true} = props, _extension_props) do | ||
| defp def_t_typespec(_props, _extension_props, transform_module_ast) when not is_nil(transform_module_ast) do | ||
| quote do | ||
| @type t() :: unquote(transform_module_ast).t(__MODULE__) |
There was a problem hiding this comment.
The biggest potential issue is that we are assuming that the function ast will always be a module. But I can't see it being anything else if you are using the library properly :)
v0idpwn
force-pushed
the
typespec-for-protobuf-transformer
branch
from
b80bf30 to
3ea9f46
Compare
v0idpwn
requested review from
whatyouhide and
ericmj
and removed request for
whatyouhide
|
I tested this in our codebases and it resolves dialyzer issues. We are using transformers for, for example, well known types. |
|
JFYI, I don't think the typespecs as they stand in this PR are good enough. I'm working on a new implementation that, although more complex, provides better typespecs. |
v0idpwn
commented
Dec 18, 2024
v0idpwn
commented
Dec 18, 2024
whatyouhide
approved these changes
Dec 18, 2024
Module is redundant. One can use __CALLER__ to retrieve the calling module env.
Wasn't able to make it work in the way I intended without significant refactoring, think the existing error is good enough for now.
v0idpwn
force-pushed
the
typespec-for-protobuf-transformer
branch
from
18fb755 to
b9c00da
Compare
v0idpwn
changed the title
typespec/1 macro to transform module behaviour
|
@whatyouhide I believe I addressed all your comments, plus I made a few simplifications. I'll go ahead and merge this :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
typespec/1macrocallback to transform module behaviour.This macro can modify the typespec for modules using it. This way, we can provide correct typespecs for modules using transformers. A "breaking" change is that transform modules can't depend on Protobuf definitions anymore. This is quite easily solvable, though.
Closes #382
Previous description, from previous implementation:
We detect that
transform_module/0was defined through a on_definition hook. We then store the module in an attribute. This attribute is later used by thebefore_compilecallback to set the typespec, which is set in the form of:Then, transform modules can implement proper typespecs.
No changes if
transform_moduleis not overriden.Closes #382