-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Fix F1 help link for error messages #46850
Conversation
} | ||
|
||
return string.Empty; | ||
return $"https://msdn.microsoft.com/query/dev16.query?appId=Dev16IDEF1&l={CultureInfo.CurrentCulture}&k=k({GetId(code)});k(DevLang-csharp)&rd=true"; |
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 don't particularly like hardcoding dev16
into the compiler, I think any link would need to be VS-version agnostic.
What would happen when we add new error codes to the compiler that aren't yet doc'd?
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 don't particularly like hardcoding dev16 into the compiler, I think any link would need to be VS-version agnostic.
I don't think it's a big deal. I even don't know how it matters if it's dev16 or dev15 or whatever version. Both redirects to the correct page.
If the docs consolidated all error messages in the same directory, we can rely on it as a different link.
What would happen when we add new error codes to the compiler that aren't yet doc'd?
It will redirect to https://docs.microsoft.com/en-us/dotnet/csharp/misc/sorry-we-don-t-have-specifics-on-this-csharp-error
(Currently, it won't do that for all error messages. That will get fixed by dotnet/docs#20069
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 this needs to go through doc team to know what links they want us to point at. @jinujoseph @kendrahavens @mikadumont do you know who would be a good contact there to ask about this?
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 don't think it's a big deal. I even don't know how it matters if it's dev16 or dev15 or whatever version. Both redirects to the correct page.
I'd prefer not to have any version at all. Errors from the C# compiler are not really version specific.
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.
@CyrusNajmabadi The docs team currently relies on f1_keywords metadata that exists in articles. Because the error messages documentation is scattered in two different folders currently, I don't think there is another way.
AFAIK, the docs team has an internal request to support a whole folder redirection, which will make it easy to have all error messages in one folder.
I'll let @BillWagner or @adegeo confirm that.
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.
OK, I will work with @kexugit about the new name and update you soon.
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.
We will deploy this change after the Ignite event ends.
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.
@Youssef1313 The change has been deployed. You can go ahead and update the PR. Thanks to @OsmondJiang 's team!
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.
Thanks a lot @kexugit and @OsmondJiang 🎉
https://msdn.microsoft.com/query/dev16.query?appId=Dev16IDEF1&l=EN-US&k=k(CS1001);k(DevLang-csharp)&rd=true
https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&l=EN-US&k=k(CS1001);k(DevLang-csharp)&rd=true
At the time of writing this, the first link works, but the second doesn't. Any idea what I'm doing wrong?
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.
src/EditorFeatures/VisualBasicTest/Squiggles/ErrorSquiggleProducerTests.vb
Show resolved
Hide resolved
@333fred Build is green. Can you review? |
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.
LGTM (commit 17). @dotnet/roslyn-compiler for another review.
@dotnet/roslyn-compiler for a second review. |
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.
LGTM Thanks (iteration 17)
Merged/squashed. Thanks @Youssef1313 ! |
Discussion started in dotnet/docs#20069 by @ChrisMaddock.
Fixes dotnet/docs#16448
Fixes #40863
Fixes #46829
Prefer squash/merge