-
Notifications
You must be signed in to change notification settings - Fork 63
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
Comments
#257 adressed the configurability of how many lines of context should be shown around labels. |
For your other point, why don't you just put a (secondary) label on the context that you want to explain? |
That is what i did, but i ended up scrapping it because: 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. |
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. |
Ah yes, its more of context inside labels. Mistake on my part 😓 |
@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? |
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: from this source codewhile(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:
|
I disagree with doing it like this, it should be something separate, maybe |
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. I'd assume that implementing that latter half at least should be pretty trivial - I might try myself at a PR for that! |
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.
The text was updated successfully, but these errors were encountered: