Indented code blocks in Hovers should use the correct language for the hover provider

[DanTup]

It would be really helpful if we could provide the language that should be used for code blocks in Hovers. Currently we can pass a string of markdown (where code will be uncoloured) or a chunk of text where the whole thing will be considered code in a language.

In Dart, comments are already markdown, so we have code like this:

/// This is a great method.
/// 
/// Here is some example code:
/// 
///     final name = "Danny";
///     print(name);
danny() {

}

The string we get is exactly like that, minus the ///. In order to get Dart highlighting code the code, we would have to split the document up, detect the code blocks and then created MarkedStrings with the language set to Dart as each one. This is quite complicated because there are other things that are indented the same way with blank lines before/after (like nested lists). It would be great if we could just set the code language and pass markdown directly.

[jrieken]

You can use the normal <triple-backtick>``<lang_name>``<newline>code<newline>``<triple-backtick>

[DanTup]

@jrieken I understand that, but it means figuring out where the code blocks are and rewriting them. Code is already doing Markdown parsing, so it seems a bit silly for us to have to do it too. All language extensions comments are probably going to have code blocks in the same language, so this seemed like it could save duplicated effort of us all writing our own markdown re-writer if comments are already markdown and contain code blocks?

[jrieken]

In Dart, comments are already markdown, so we have code like this:

Maybe it's a formatting issue with GH but I really don't see any markdown in that sample, esp no code-block

[DanTup]

The lines indented by 4 spaces are code, but maybe it wasn't a very clear example.

Here's a better (real) example; the Future<T> class. This is the real dartdoc from the class. We get this string (stripped of the comment markers) and do some basic tweaking of it. There are a bunch of code samples inside it. Code's rendering in hovers does correctly interpret these things indented with 4-spaces as code, but it doesn't know that they're dart so they don't get syntax highlighting.

Since this comment isn't written by us (extension author) but the end user, we can't ask them to add backticks (and it's also not the convention, as in Dart we know it's Dart). Surrounding the code blocks with backticks ourselves (in the hover provider) is possible but does mean we'll have to parse the Markdown on some level (because nested lists look a little like code blocks; they have blank lines before/after and are indented with 4 spaces).

I'm not against rewriting the markdown ourselves, but I think a more elegant solution would be if we could just tell Code that all code blocks in this markdown should be considered Dart. If you don't think that's a good feature; that's fine; I just wanted to suggest it before I embarked on Markdown-parsing :-)

Hope this is clearer!

/**
 * An object representing a delayed computation.
 *
 * A [Future] is used to represent a potential value, or error,
 * that will be available at some time in the future.
 * Receivers of a [Future] can register callbacks
 * that handle the value or error once it is available.
 * For example:
 *
 *     Future<int> future = getFuture();
 *     future.then((value) => handleValue(value))
 *           .catchError((error) => handleError(error));
 *
 * A [Future] can complete in two ways:
 * with a value ("the future succeeds")
 * or with an error ("the future fails").
 * Users can install callbacks for each case.
 * The result of registering a pair of callbacks is a new Future (the
 * "successor") which in turn is completed with the result of invoking the
 * corresponding callback.
 * The successor is completed with an error if the invoked callback throws.
 * For example:
 *
 *     Future<int> successor = future.then((int value) {
 *         // Invoked when the future is completed with a value.
 *         return 42;  // The successor is completed with the value 42.
 *       },
 *       onError: (e) {
 *         // Invoked when the future is completed with an error.
 *         if (canHandle(e)) {
 *           return 499;  // The successor is completed with the value 499.
 *         } else {
 *           throw e;  // The successor is completed with the error e.
 *         }
 *       });
 *
 * If a future does not have a successor when it completes with an error,
 * it forwards the error message to the global error-handler.
 * This behavior makes sure that no error is silently dropped.
 * However, it also means that error handlers should be installed early,
 * so that they are present as soon as a future is completed with an error.
 * The following example demonstrates this potential bug:
 *
 *     var future = getFuture();
 *     new Timer(new Duration(milliseconds: 5), () {
 *       // The error-handler is not attached until 5 ms after the future has
 *       // been received. If the future fails before that, the error is
 *       // forwarded to the global error-handler, even though there is code
 *       // (just below) to eventually handle the error.
 *       future.then((value) { useValue(value); },
 *                   onError: (e) { handleError(e); });
 *     });
 *
 * When registering callbacks, it's often more readable to register the two
 * callbacks separately, by first using [then] with one argument
 * (the value handler) and using a second [catchError] for handling errors.
 * Each of these will forward the result that they don't handle
 * to their successors, and together they handle both value and error result.
 * It also has the additional benefit of the [catchError] handling errors in the
 * [then] value callback too.
 * Using sequential handlers instead of parallel ones often leads to code that
 * is easier to reason about.
 * It also makes asynchronous code very similar to synchronous code:
 *
 *     // Synchronous code.
 *     try {
 *       int value = foo();
 *       return bar(value);
 *     } catch (e) {
 *       return 499;
 *     }
 *
 * Equivalent asynchronous code, based on futures:
 *
 *     Future<int> future = new Future(foo);  // Result of foo() as a future.
 *     future.then((int value) => bar(value))
 *           .catchError((e) => 499);
 *
 * Similar to the synchronous code, the error handler (registered with
 * [catchError]) is handling any errors thrown by either `foo` or `bar`.
 * If the error-handler had been registered as the `onError` parameter of
 * the `then` call, it would not catch errors from the `bar` call.
 *
 * Futures can have more than one callback-pair registered. Each successor is
 * treated independently and is handled as if it was the only successor.
 *
 * A future may also fail to ever complete. In that case, no callbacks are
 * called.
 */
abstract class Future<T> {
  // <snip>
}

[jrieken]

Thanks for clarifying - I actually didn't know that you can format code blocks using space indentation... Now it makes sense to me.

Thinking of a way to achieve this... We have this and it should be save to access the modeId of the current model in case the value is missing. I'll investigate

[jrieken]

That means we won't allow to set the default (unless a really good idea for that comes up) but simply take the context as default

[DanTup]

Ah sorry, I should've provided a better example to start and maybe it would've been more obvious what I meant!

I don't fully understand the last comment; do you mean you'll infer it from the language of the HoverProvider rather than letting us set it? (if so, that seems perfect to me!)

[jrieken]

Yeah - it would be the default when no language is set. Slightly on thin ice with that cos it changes existing behaviour but I'll give it a shot beginning of September

[DanTup]

Awesome; thank you! 👍

[octref]

I'm guessing this isn't the expected behavior, as dart code doesn't syntax highlight either hover or markdown comments in code...Or is it because you haven't updated Dart-Code to utilize this functionality?
@DanTup

@jrieken How could I verify this works?

[DanTup]

@octref It did work when I tested it in Insiders 18 days ago (see Dart-Code/Dart-Code#160 (comment)) but I haven't tested it recently. I'll make a note to give it another look tomorrow when I'm at a PC.

[DanTup]

@octref Indeed, it seems broken now; the colouring is all gone (was like that before I updated today - was probably a week old? and with todays version).

[octref]

@DanTup It's moved to October.

@chrmarti As this is not working and moved to Oct I'm removing verification-needed to get it out of the query.

[jrieken]

@alexandrudima Is this due to the mode/tokenization changes you made?

[jrieken]

Yeah - looks like a regression from 04f43c3 where the fallback to the editor mode id got lost

[alexdima]

@jrieken reopening to assess if this is a possible candidate for the stable branch

[egamma]

Removing the candidate for September stable since it is not a regression.

[egamma]

Fixed in the latest.

[octref]

Hover works, code in comment doesn't.

I'm looking at this file: https://github.com/angular/angular.dart/blob/master/lib/formatter/filter.dart

Also wondering, is this something we should do? In the case of angular.dart there are also a lot of HTML code in comment. Marking them as dart doesn't make sense, but providing a custom way for author to say which lang their code is in doesn't sounds good either.

[alexdima]

@octref How should we know that half the comment is in dart and half the comment is in html ?

[octref]

@alexandrudima I thought the comment section are supposed to behave like hover, which marks everything as dart, in this dart file.

I think we should only support github-style 3-backstick code blocks in comment.

[jrieken]

This issue was about falling back to the language currently being shown in the editor if the markdown code block doesn't specify one.

[chrmarti]

Using @DanTup's dart extension this works for me. Looking at dart-lang/sdk's future.dart with an untrained eye. Marking verified.