dotnet / dotnet-api-docs

.NET API reference documentation (.NET 5+, .NET Core, .NET Framework)
https://docs.microsoft.com/dotnet/api/
Other
719 stars 1.56k forks source link

Incorrectly rendered xref link #3586

Closed svick closed 3 years ago

svick commented 7 years ago

The remarks for MethodBase.Invoke(Object, Object[]) contain an xref link to the other overload (MethodBase.Invoke(Object, BindingFlags, Binder, Object[], CultureInfo)), but it renders as:

[Invoke(Object, BindingFlags, Binder, Object\<xref:System.Reflection.MethodBase.Invoke%28System.Object%2CSystem.Reflection.BindingFlags%2CSystem.Reflection.Binder%2CSystem.Object%5B%5D%2CSystem.Globalization.CultureInfo%29>

The source seems to be fine, so my guess is that this is some issue with converting xref links to markdown, likely caused by the presence of encoded square brackets in the link.

mairaw commented 7 years ago

@GuardRex can you take a look at this one? The issue reported by @svick seems to be fixed. However, there are some other broken links on that page that I'm guessing is because ECMAXML sections are using MarkDown syntax.

For example: image

guardrex commented 7 years ago

Opened internally: TFS#: 1028877

guardrex commented 7 years ago

@svick Since it doesn't look like the markdown parser is running in those spots, a workaround is to use ...

<format type="text/html">
  <a href="http://go.microsoft.com/fwlink/?LinkID=247912">.NET for Windows Store apps</a>
</format>

capture

We're waiting for confirmation on whether or not that's a valid approach ... or just another RexHack:tm: :grinning:.

guardrex commented 7 years ago

Thanks to [@]dend, we have our answer. The correct way to set them up outside of content that's parsed as markdown is to use this format ...

<see href="http://microsoft.com">Microsoft</see>

I'll get 'um fixed up soon.

guardrex commented 7 years ago

@mairaw The script is coming along well with md and http links, but there are two link types that require a solution.

Some of the links outside of markdown remarks are ~/includes links and some are ~/samples links. I'll show one example of each below, but there are 497 of these! Geebers McScreebers!

There are 299 includes links to fix

Example: xml\Microsoft.VisualBasic.Compatibility.VB6\BaseControlArray.xml

https://github.com/dotnet/docs/blob/master/xml/Microsoft.VisualBasic.Compatibility.VB6/BaseControlArray.xml#L499

On Line 499, there are three ~/includes inside a <returns> ...

<returns>
  ...
  <block subset="none" type="note">
    <para>
      Functions and objects in the <see cref="N:Microsoft.VisualBasic.Compatibility.VB6" /> 
      namespace are provided for use by the tools for upgrading from Visual Basic 6.0 to 
      [!INCLUDE[vbprvb](~/includes/vbprvb-md.md)]. In most cases, these functions and 
      objects duplicate functionality that you can find in other namespaces in the 
      [!INCLUDE[dnprdnshort](~/includes/dnprdnshort-md.md)]. They are necessary only 
      when the Visual Basic 6.0 code model differs significantly from the 
      [!INCLUDE[dnprdnshort](~/includes/dnprdnshort-md.md)] implementation.
    </para>
  </block>
</returns>

That topic is here: https://docs.microsoft.com/en-us/dotnet/api/microsoft.visualbasic.compatibility.vb6.basecontrolarray.getcontrolinstancetype?view=netframework-4.7#Microsoft_VisualBasic_Compatibility_VB6_BaseControlArray_GetControlInstanceType

... and it renders like this ...

capture

There are 198 samples links to fix

Example: xml\System\Convert.xml

https://github.com/dotnet/docs/blob/master/xml/System/Convert.xml#L235

On Lines 235, 236, 240, and 241, a few ~/samples links ...

~/samples/snippets/csharp/VS_Snippets_CLR_System/system.convert.changetype/cs/changetype_enum2.cs#5

<block subset="none" type="usage">
  <para>
    ...
    [!code-csharp[System.Convert.ChangeType#5](~/samples/snippets/csharp/VS_Snippets_CLR_System/system.convert.changetype/cs/changetype_enum2.cs#5)]
    [!code-vb[System.Convert.ChangeType#5](~/samples/snippets/visualbasic/VS_Snippets_CLR_System/system.convert.changetype/vb/changetype_enum2.vb#5)]
    ...
    [!code-csharp[System.Convert.ChangeType#7](~/samples/snippets/csharp/VS_Snippets_CLR_System/system.convert.changetype/cs/changetype_nullable.cs#7)]
    [!code-vb[System.Convert.ChangeType#7](~/samples/snippets/visualbasic/VS_Snippets_CLR_System/system.convert.changetype/vb/changetype_nullable.vb#7)]
  </para>
</block>

(btw ☝️ is that ok? ... doesn't look good ... can <block> be a child element of <Docs>? PTAL)

?

I can fix the ~/docs and http links easily enough because those are just casts to the <see> format.

For ~/includes, I could look up the values and inject the text right in there for them.

For the ~/samples, idk ... Do I need to analyze a bit more and see if these <block> elements are even in the right spots? If they're not even supposed to be there (i.e., childs of <Docs> elements), the simple solution will be to just 🔪 them out in these spots.

cc/ @dend

guardrex commented 7 years ago

Blocking until Steps 1-5 on https://github.com/dotnet/docs/issues/2580 are merged.

guardrex commented 7 years ago

@mairaw Since dotnet/docs#2580 can't be finished by Monday, I'll have to unassign this one and hand it over.

mairaw commented 6 years ago

Since this is in the ECMAXML. it shouldn't contain MarkDown text. Given that we're trying to redirect MSDN soon, this should be handled soon.

ChrisMaddock commented 6 years ago

@mairaw - I see this issue's currently assigned to you, is it open to community contributions, or are you working on something to cover it all? 😄

Just seems like another fun little script challenge. 🙂

mairaw commented 6 years ago

@ChrisMaddock I barely got started on that and with .NET Core 2.1 RTM approaching, I don't think I'll be able to focus on this one for the next couple of weeks. I'll put it back to the backlog.

BillWagner commented 6 years ago

Tested and it's now rendering correctly. Closing

@svick if you see a different display, reopen this issue.

mairaw commented 6 years ago

We still lots of those, not the one from the first description. I'll a new repro here.

mairaw commented 6 years ago

Actually that link is still for some of the issues we have with broken links: image

ChrisMaddock commented 6 years ago

Hey @rpetrusha @mairaw,

Can I just check this is still an issue you want to receive PR's on? On the last PR (https://github.com/dotnet/dotnet-api-docs/pull/368), Ron said:

There's also an additional consideration, that there's been some discussion of migrating XML to markdown, in which case much of this work would probably be done by the migration process.

Was there an outcome to that conversation? I don't want to waste your time reviewing things which would shortly be corrected automatically anyway!

Also, Maira - we were talking on the same PR about whether to use ~/ qualified, or full links in these sections. The last comment on that was...

I know they work for docs, not sure if they work with the API browser and were still trying to solve some issues preventing us from indexing our APIs in the API browser right now, so it'd take a while to confirm. Let's use the docs,microsoft.com links now and we can switch to relative links once we can confirm.

Is there any update on that, or shall we keep using full links for now? 🙂

ChrisMaddock commented 6 years ago

image

I've done some work replacing the markdown links in these places, but just noticed it also looks like <see cref's aren't rendered in summary tags either.

Should these also all be converted to <see href> links? Or is there a plan to update the site to support cref's in this position? If they need updating, that's probably big enough to be a separate issue...

ChrisMaddock commented 6 years ago

Ah - I see the above is https://github.com/MicrosoftDocs/feedback/issues/637.

I presume we're hoping for a change on the platform, rather than changing all the content?

mairaw commented 5 years ago

Unfortunately, I think that was a migration issue. Because that content should have been moved with see tags instead of MarkDown links. Yes, I'm hoping they can fix that but certainly changing the links fixes the problem we're seeing now.

I've asked them to reindex the content again after your changes and now the link text appears as expected. e.g. https://docs.microsoft.com/en-us/dotnet/api/?term=EventLogPropertySelector

daveyostcom commented 5 years ago

Also here.

carlossanlop commented 4 years ago

@guardrex have you had a chance to get all the instances of this issue fixed?

guardrex commented 4 years ago

@carlossanlop ... I moved to the ASP.NET Core docs a few years ago. I'm not sure where this stands today

gewarren commented 3 years ago

I think this issue can be closed. I checked and the ~/includes and ~/samples links are rendering correctly in the few that I checked.

There's a separate issue in the GetControlInstanceType type, where this information probably shouldn't be in the return block, but I don't think it's worth fixing.