Skip to content

Add documentation for CallStack "caller" method#4702

Closed
pglombardo wants to merge 4 commits intocrystal-lang:masterfrom
pglombardo:documentation
Closed

Add documentation for CallStack "caller" method#4702
pglombardo wants to merge 4 commits intocrystal-lang:masterfrom
pglombardo:documentation

Conversation

@pglombardo
Copy link
Copy Markdown

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.

Comment thread src/callstack.cr Outdated
@pglombardo
Copy link
Copy Markdown
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.

Comment thread src/callstack.cr Outdated
Comment thread src/callstack.cr Outdated
Comment thread src/callstack.cr
@pglombardo
Copy link
Copy Markdown
Author

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

@pglombardo
Copy link
Copy Markdown
Author

Updated screenshots:

screen shot 2017-07-11 at 22 36 08

screen shot 2017-07-11 at 22 35 49

Comment thread src/callstack.cr Outdated
@bew
Copy link
Copy Markdown
Contributor

bew commented Jul 11, 2017

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

@pglombardo
Copy link
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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.

Comment thread src/callstack.cr
require "debug/dwarf"
{% end %}

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

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
Copy Markdown
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
Copy Markdown
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.

7 participants