Skip to content

Add :doc command in REPL to show documentation #4669

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 9, 2018

Conversation

Duhemm
Copy link
Contributor

@Duhemm Duhemm commented Jun 15, 2018

Depends on #4461 and #4648

This command is used to display the documentation associated to the
given expression. For instance:

scala> /** A class */ class A
scala> /** An object */ object O { /** A def */ def foo = 0 }
scala> :doc new A
/** A class */
scala> :doc O
/** An object */
scala> :doc O.foo
/** A def */

(Only the last 2 commits are relevant for this PR)

Duhemm added 2 commits June 29, 2018 15:06
This command is used to display the documentation associated to the
given expression. For instance:

```scala
scala> /** A class */ class A
scala> /** An object */ object O { /** A def */ def foo = 0 }
scala> :doc new A
/** A class */
scala> :doc O
/** An object */
scala> :doc O.foo
/** A def */
```
implicit val ctx: Context = state.context

/** Extract the (possibly) documented symbol from `tree` */
def extractSymbol(tree: tpd.Tree): Symbol = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this is needed? Can you give an example

if (stat.tpe.isError) stat.tpe.show
else {
val doc =
ctx.docCtx.flatMap { docCtx =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe in the REPL we should always have a docCtx

* Return also the symbols that `symbol` overrides`.
*/
def pickSymbols(symbol: Symbol): Iterator[Symbol] = {
val selectedSymbol = {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: unnecessary braces

}

typeCheck(expr).map {
case v @ ValDef(_, _, Block(stats, _)) if stats.nonEmpty =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you maybe change typeOf to return a Result[Type] and use typeOf instead of typeCheck

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're not interested in getting the type of the expression, but rather the typed AST so that we can extract the symbol that we're interested in from it:

scala> /** bar */ def foo: Int = 2
scala> :doc foo // prints: /** bar */

I want to get the symbol of foo so that I can retrieve its documentation; I don't care about its type here.

* Adapt `symbol` so that we get the one that holds the documentation.
* Return also the symbols that `symbol` overrides`.
*/
def pickSymbols(symbol: Symbol): Iterator[Symbol] = {
Copy link
Contributor

@allanrenucci allanrenucci Jun 29, 2018

Choose a reason for hiding this comment

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

Why is this needed in the REPL but not the IDE? In the IDE you do:

val symbol = Interactive.enclosingSourceSymbol(trees, pos)
val docComment = ctx.docCtx.flatMap(_.docstring(symbol))


@Test def docOfInherited =
fromInitialState { implicit s => run("class C { /** doc */ def foo = 0 }") }
.andThen { implicit s => run("object O extends C") }
Copy link
Contributor

Choose a reason for hiding this comment

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

same (use a single run)

|/** doc1 */ def foo(x: String): String = x + "foo"
|}""".stripMargin)
}
.andThen { implicit s =>
Copy link
Contributor

Choose a reason for hiding this comment

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

same (use a single run)

assertEquals("/** doc0 */", storedOutput().trim)
s
}
.andThen { implicit s =>
Copy link
Contributor

Choose a reason for hiding this comment

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

same (merge andThen block)

| abstract class Companion { /** doc0 */ def bar: Int }
| /** companion */ def foo: Companion
|}""".stripMargin)
.andThen { implicit s =>
Copy link
Contributor

Choose a reason for hiding this comment

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

same (use a single run)

assertEquals("/** companion */", storedOutput().trim)
s
}
.andThen { implicit s =>
Copy link
Contributor

Choose a reason for hiding this comment

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

same (merge andThen block)

@allanrenucci allanrenucci assigned Duhemm and unassigned allanrenucci Jun 29, 2018
Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

Code LGTM, but I'm not convinced by the newly introduced syntax to query the doc. Let's discuss this in a meeting

Copy link
Contributor

@allanrenucci allanrenucci left a comment

Choose a reason for hiding this comment

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

Let's merge this for now and build on top of this

@allanrenucci allanrenucci merged commit 3c3130d into scala:master Jul 9, 2018
@allanrenucci allanrenucci deleted the topic/tasty-doc-repl branch July 9, 2018 15:08
@smarter smarter mentioned this pull request Apr 30, 2019
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.

2 participants