Skip to content

Allow to prune metadata while escaping #7949

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

Merged
merged 4 commits into from
Jul 21, 2018
Merged

Allow to prune metadata while escaping #7949

merged 4 commits into from
Jul 21, 2018

Conversation

josevalim
Copy link
Member

@josevalim josevalim commented Jul 20, 2018

We use this option in ExUnit as we are escaping
the code to eventually convert it to a string
representation and therefore the metadata is not
relevant.

Closes #7947.

We use this option in ExUnit as we are escaping
the code to eventually convert it to a string
representation and therefore the metadata is not
relevant.
@josevalim josevalim force-pushed the jv-prune-metadata branch from 4526e7f to b4c3ea9 Compare July 20, 2018 21:58
@bitwalker
Copy link
Contributor

Just wanted to chime in here and let you know that this PR solves the problem I was seeing - the tests that were previously failing due to :badfun now succeed!

@michalmuskala
Copy link
Member

This looks good, but I'm still not sure when I would use something like this writing code in Elixir - could you give some examples and explanation why it's necessary, @josevalim?

@josevalim
Copy link
Member Author

@michalmuskala done! please let me know if it is clearar now!

As an example, `ExUnit` stores the AST of every assertion, so when
an assertion fails we can show code snippets to users. Without this
option, each time the test module is compiled, we get a different
MD5 of the module byte code, because the AST contains metadata
Copy link
Member

Choose a reason for hiding this comment

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

I think there is something missing in this part:

because the AST contains metadata specific, such as counters, specific the compilation environment.

Copy link
Member Author

Choose a reason for hiding this comment

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

One word too many! Tks, fixed!

Copy link
Member

Choose a reason for hiding this comment

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

😬

But, might a ”to” be missed too? Right now it reads “because the AST contains metadata specific the compilation environment”, should it be “because the AST contains metadata specific to the compilation environment”?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch again! :D

@michalmuskala
Copy link
Member

Looks good to me :)

@josevalim josevalim merged commit 34befaa into master Jul 21, 2018
@josevalim
Copy link
Member Author

❤️ 💚 💙 💛 💜

@josevalim josevalim deleted the jv-prune-metadata branch July 21, 2018 10:56
@arjan
Copy link
Contributor

arjan commented Jul 24, 2018

@josevalim any chance this gets backported into 1.6?

@josevalim
Copy link
Member Author

@arjan the feature or the fix?

@arjan
Copy link
Contributor

arjan commented Jul 24, 2018

Good question, b4c3ea9 I guess, it's for this library. cc @bitwalker

@bitwalker
Copy link
Contributor

@josevalim I'd be curious to know which release this would fall in, and if backporting to 1.6 is an option (assuming it's in 1.7), that'd open up the ability to use libraries like the one linked with current applications which may not upgrade for some time, though I'm not too concerned about that if it'll be in 1.7. If it's not going to be in 1.7 either, that'd be a problem for my library, but if there are problems with dropping it that soon, I just need to provide people with instructions on how to use master in the meantime.

@josevalim
Copy link
Member Author

josevalim commented Jul 25, 2018 via email

@bitwalker
Copy link
Contributor

Thanks José! Sounds good to me :)

josevalim added a commit that referenced this pull request Jul 25, 2018
We use this option in ExUnit as we are escaping
the code to eventually convert it to a string
representation and therefore the metadata is not
relevant.

Signed-off-by: José Valim <jose.valim@plataformatec.com.br>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants