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

Remove stacktrace warning #242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove stacktrace warning #242

wants to merge 2 commits into from

Conversation

felipesere
Copy link
Collaborator

Using STACKTRACE is not really an option due to it requiring a
try/catch block, which we'd have to possibly add to every koan.

Side-step the issue by calling into Erlang

lib/execute.ex Outdated
@@ -40,7 +40,7 @@ defmodule Execute do

defp expand(error, module) do
{file, line} =
System.stacktrace()
:erlang.get_stacktrace()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will have to be rebased due to a change in #251

@josevalim
Copy link

FWIW, this is not going to work on recent OTP versions. Erlang/OTP 23 always returns an empty list for stacktrace and the next version will fully remove the function.

@felipesere
Copy link
Collaborator Author

@josevalim @iamvery My current thinking is that we can avoid going through the stacktrace to find file & line if we just capture them in the generated koan function using __ENV__.file and __ENV__.line in the first place.

I'll try a PR this evening and update this :)

@felipesere felipesere force-pushed the stacktrace-warnings branch from 67198dc to 2c4e1f5 Compare June 19, 2020 07:44
@felipesere
Copy link
Collaborator Author

I'm quite happy with this new approach.
If an error happens within a Koan, I grab __ENV__.line and __ENV__.file and report it back up as part of the error. That way, there is no need to walk the stacktrace anymore!

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.

3 participants