-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Improve quickinfo/signature/completions baselines #52308
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
Improve quickinfo/signature/completions baselines #52308
Conversation
Add a copy with of the file at the top with annotations, like find-all-ref and rename baselines. I'm not 100% sure about the formatting, but I don't think it's too overbearing; for the few files with lots of tests, more decoration is helpful. I don't know whether sticking to jsonc syntax is worthwhile either. I think the '// ' prefix hurts readability but might be worthwhile in some future world where we need to query our baselines for information.
Lots of duplicated code; I'll improve that later.
Even more duplicated code.
And start refactoring all 3 baseline functions to a single one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big improvement. I don’t have a problem with this format with the leading slashes, but one thing you can consider is using a real file extension (.baseline.jsonc
for example) if you want to control the syntax highlighting. I've used Markdown for some, which has the advantage of embedding languages.
tests/baselines/reference/completionDetailsOfContextSensitiveParameterNoCrash.baseline
Show resolved
Hide resolved
// property = "foo" | ||
// | ||
// | ||
// ^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: lots, but not all, of these have 2 extra blank lines. Is that easy to fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what you're seeing is from the source, not the new formatting:
export class C implements I {
property = "foo"
/*1*/
It's easy to fix but hard to know whether I've fixed them all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I don’t have a problem if it was from the source. I just assumed it was a baseline issue that it made it look like the source was doing weird stuff.
Add a copy of the file at the top with tooltip-like annotations, a bit like find-all-ref and rename baselines.
I'm not 100% sure about the formatting, but I don't think it's too overbearing; for the few files with lots of tests, more decoration is helpful.
I don't know whether sticking to jsonc syntax is worthwhile either. I think the
//
prefix hurts readability but might be worthwhile in some future world where we need to query our baselines for information.