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

Change case order in SyntaxSugar.transform_expression/2 to handle already expanded AST #43

Merged
merged 3 commits into from
May 7, 2024

Conversation

hmy3743
Copy link
Contributor

@hmy3743 hmy3743 commented Apr 15, 2024

This is Pull request to fix bug happened in my code

typed_schema is not work properly inside custom macro code. AST is already expanded before transform_expression called. So :__block__ nested function call is not work

below code is summary of my macro code

defmacro typed_schema(name, do: {function_name, ctx, args}) do
  fields = {function_name, ctx, Enum.map(args, &put_null_false/1)}

  quote do
    @timestamps_opts [type: :utc_datetime]

    TypedEctoSchema.typed_schema unquote(name) do
      unquote(fields)

      timestamps()
    end
  end
end

Thanks :)

@dvic
Copy link
Collaborator

dvic commented Apr 24, 2024

@hmy3743 I fixed the CI, can you please rebase this PR on the latest master branch?

@hmy3743
Copy link
Contributor Author

hmy3743 commented May 3, 2024

Sorry for the delay :) I've rebased and pushed the branch.

@dvic
Copy link
Collaborator

dvic commented May 3, 2024

LGTM! Can you add a basic test case in test/typed_ecto_schema_test.exs so we don't miss this in future refactorings?

@dvic
Copy link
Collaborator

dvic commented May 7, 2024

Thanks for the test, is there a way for you to add the previous code (that failed to compile) in the test instead? just with some basic assertion that checks we compiled the right thing :)

@dvic dvic merged commit 51c4b7f into bamorim:master May 7, 2024
9 checks passed
@dvic
Copy link
Collaborator

dvic commented May 7, 2024

Thanks!

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