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

Add documentation for CallStack "caller" method #4702

Closed
wants to merge 4 commits into from
Closed

Add documentation for CallStack "caller" method #4702

wants to merge 4 commits into from

Conversation

pglombardo
Copy link

Per convo in gitter, it would be helpful to have this method (caller) exposed in the API documentation.

This PR adds a short method explanation and adds callstack.cr to the documentation build process.

src/callstack.cr Outdated Show resolved Hide resolved
@pglombardo
Copy link
Author

Updated per feedback. The documentation now looks like this once generated:

screen shot 2017-07-11 at 20 34 04

screen shot 2017-07-11 at 20 35 09

I'll stick to this format/language for future PRs.

src/callstack.cr Outdated Show resolved Hide resolved
src/callstack.cr Outdated Show resolved Hide resolved
src/callstack.cr Outdated Show resolved Hide resolved
@pglombardo
Copy link
Author

Updated per 2nd round of feedback. Keep it coming :-)

@pglombardo
Copy link
Author

Updated screenshots:

screen shot 2017-07-11 at 22 36 08

screen shot 2017-07-11 at 22 35 49

src/callstack.cr Outdated Show resolved Hide resolved
@bew
Copy link
Contributor

bew commented Jul 11, 2017

Those 5 lines additions will be perfect for your first contribution 😆

@pglombardo
Copy link
Author

I should have read the doc guidelines a bit more thoroughly but I also considered that this might be a ritual hazing for first time contributor to the repo? ;-)

@RX14
Copy link
Contributor

RX14 commented Jul 11, 2017

In all honestly, having this method at all is a bad idea because it encourages people to reply on stacktraces to be accurate. And in release mode, they absolutely are not. There is no way to tell which function you were called from in release mode.

@bew
Copy link
Contributor

bew commented Jul 11, 2017

@RX14 That's right, I think it should at least be mentioned more clearly in the docs that the method is almost useless in release mode.

@pglombardo
Copy link
Author

Ok - can someone suggest a sentence or two with a possible hint as to why this is the case?

i.e. "Note: In release mode, the usefulness of the callstack is questionable due to blah blah blah"

s/blah blah blah/<something smart>/g

Whether this method should exist at all may be true but is probably beyond the scope of this 5 line code doc PR.

@RX14
Copy link
Contributor

RX14 commented Jul 11, 2017

@pglombardo it may be beyond the scope of this PR but there's little point landing this PR if we have a consensus that the method should be removed.

@straight-shoota
Copy link
Member

That's true... what's the use case for caller? It's probably not that important to have this as a shortcut for CallStack.new (which you can always call if you need a call stack for some reason).

@pglombardo
Copy link
Author

If we're speaking about what the best API approach would be for this functionality, I would think that a class method on CallStack would be the most logical (inspired from Ruby Kernel.caller).

Callstack.caller

The alternative is to call CallStack.new.printable_backtrace which also works but isn't as nice.

I can close this PR and make a separate one with these changes (and the method docs) if no one objects.

@ysbaddaden
Copy link
Contributor

CallStack is internal implementation detail, and musn't be documented publicly.

caller is a simple method to return/printout a stack, without having to raise an exception. I use it every so often.

Release mode may inline calls, just like development inlines blocks (non captured ones). Backtrackes are never accurate, but still helpful and never wrong, whatever the optimizations.

@@ -17,6 +17,10 @@ require "callstack/lib_unwind"
require "debug/dwarf"
{% end %}

# Returns the execution stack (a backtrace) of the current
Copy link
Contributor

Choose a reason for hiding this comment

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

As per @ysbaddaden's comment, how about we clarify that this is a human readable execution stack and add a note that it can be used while debugging. Then I think it's ready to merge.

@jhass
Copy link
Member

jhass commented Jun 3, 2019

I think we still want this, in case anybody wants to pick it up and finish :)

@straight-shoota
Copy link
Member

Resolved by #9076

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants