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 an option to render more context around lines. #278

Open
RDambrosio016 opened this issue Aug 27, 2020 · 11 comments
Open

Add an option to render more context around lines. #278

RDambrosio016 opened this issue Aug 27, 2020 · 11 comments

Comments

@RDambrosio016
Copy link
Contributor

When you have a diagnostic and a couple of single labels explaining context, it is sort of confusing to see where the error actually is in the code at a glance. It would be great if there was a "put these many source lines around each label. And ideally a "include this span in the diagnostic too, but without a label.

@Johann150
Copy link
Collaborator

#257 adressed the configurability of how many lines of context should be shown around labels.

@Johann150
Copy link
Collaborator

For your other point, why don't you just put a (secondary) label on the context that you want to explain?

@RDambrosio016
Copy link
Contributor Author

That is what i did, but i ended up scrapping it because:
The diagnostic references two nodes in a statement, i just want 3 - 5 lines around each label's location to tell the user "this is where the wrong stuff is" without having them have to go to the line. But this gets very cluttered in real world code. For example a try statement in js code (which is what i am doing) is usually decently large. And i reference something in the try {} block and the finally {} block, both of which may be large.

Overall, having another colored line adds clutter to the diagnostic, Your eyes should drift straight to the two places which are labelled, and you should instantly know where that code is. Having more things for the user to consider is ugly.

@RDambrosio016
Copy link
Contributor Author

RDambrosio016 commented Aug 27, 2020

I think i will use the pre-release version for now with the context lines option for now. Then i will just update once it is released 👍

Edit: realized this is for multiline labels only for now, which isnt exactly what i need. hope an option for single labels could be added in the future.

@Johann150
Copy link
Collaborator

Ah yes, its more of context inside labels. Mistake on my part 😓

@ratmice
Copy link
Collaborator

ratmice commented Sep 3, 2020

@RDambrosio016 Any chance you could post an example (of the output generated) for such a case where either what you're trying to achieve or how it looks using secondary labels?

@RDambrosio016
Copy link
Contributor Author

Sure, this is what it normally looks like without context. You cant really tell where exactly in the code it is unless you go back and look at the line numbers. This is how it is with a secondary label, i personally dont like it, and if the catch for example had a ton of stuff in it it would end up looking horrid. with context

Ideally i would just be able to say "3 lines of context around each label".

@Johann150
Copy link
Collaborator

Johann150 commented Sep 3, 2020

If you took the approach with a secondary label, you could adjust how many lines of context within the secondary label are shown (as I mentioned above). This would resolve the case of "if the catch had a ton of stuff in it". Your personal preference aside, I think this is a good alternative. Here is an example of how it would look using the current version:
image

from this source code
while(true){
    try{
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        foo();
        if(condition()){
            continue;
        }
    }catch{
        bar();
        bar();
        bar();
        bar();
        bar();
        bar();
    }finally{
        break;
    }
}

This example shows me some other problems though:

  1. Something got lost on the way of resolving File column/line should consistently point to the first primary label instead of the first label #259 with locus at first primary label #260 and the locus is at the earliest label again (notice the error is supposedly on line 2 instead of line 25).
  2. What you mentioned already: The context should also work for secondary labels and single line labels. But I think it would be enough to enable this feature only if the label is inside another multiline label.
  3. Another thing that might make sense is to add something like tertiary labels which would look better than my "This is the finally block." label. They should not be rendered but just force the specific line to be shown.

@RDambrosio016
Copy link
Contributor Author

Another thing that might make sense is to add something like tertiary labels which would look better than my "This is the finally block." label. They should not be rendered but just force the specific line to be shown.

I disagree with doing it like this, it should be something separate, maybe Context, a diagnostic would have a Vec<Context>. A context basically just holds a range and lines in those ranges will be rendered but without a label or coloring.

@Johann150
Copy link
Collaborator

I found that there already is #29, which is I think more close to what you were trying to get at. In that light I think suggestion № 3 is not necessary.

№ 1 has already been solved in #282.

@elkowar
Copy link
Contributor

elkowar commented Aug 2, 2021

I'd really love a solution that allows me to both specify a range of lines to display without requiring a label around it (i.e. the proposed "context" field on a diagnostic), as well as allowing me to specify a default amount of lines of context for any label.
That latter option should imo just be part of the config struct, such that one can set
config.context_before = 2 and config.context_after = 2 so specify "show at least 2 lines before and two lines after my label as context.

I'd assume that implementing that latter half at least should be pretty trivial - I might try myself at a PR for that!

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

No branches or pull requests

4 participants