-
Notifications
You must be signed in to change notification settings - Fork 868
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
feat: view source for assemblies using sourcelink #8548
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #8548 +/- ##
==========================================
- Coverage 75.22% 75.18% -0.04%
==========================================
Files 645 649 +4
Lines 23637 23837 +200
==========================================
+ Hits 17782 17923 +141
- Misses 5855 5914 +59
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
// GitHub | ||
return Regex.Replace( | ||
rawUrl, | ||
@"^https://raw\.githubusercontent\.com/([^/]+)/([^/]+)/([^/]+)/(.+)$", | ||
string.IsNullOrEmpty(branch) ? "https://github.com/$1/$2/blob/$3/$4" : $"https://github.com/$1/$2/blob/{branch}/$4"); |
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.
Can this be customised to support hosting services other than GitHub, by adding JavaScript code to the theme or template? Like the "Improve this Doc" link can.
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.
That is a valid ask. I need some time to think through the extensibility model to see where this fits so it is not included in the first place. The template JavaScript isn't always the best way of customization and today's template extensibility requires a lot of copy n paste and is error prone. I hated it when forking the modern template and having to support a combination of default/modern/statictoc/memberpage templates.
The other approach is to expose a set of regex-based configs for mutate git URLs.
public void Dispose() | ||
{ | ||
GC.SuppressFinalize(this); | ||
_peReader.Dispose(); | ||
_pdbReaderProvider.Dispose(); | ||
} | ||
|
||
~SourceLinkProvider() | ||
{ | ||
Dispose(); | ||
} |
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.
A finalizer generally should not call Dispose() on other objects. If those own unmanaged resources, they should have their own finalizers.
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.
True
Computes source URL of a symbol using source link: When an assembly is built with source link enabled, and a PDB file exists next to the assembly, docfx computes the source URL for "view source" and cross reference of a symbol using the source link info in the PDB file.
PDBs usually exist for the entry assemblies, so "view source" typically works. But PDB files are usually missing for NuGet dependencies, so cross referencing rarely works unless we have NuGet/Home#9667
#8567