Fix Affected APIs section

[Youssef1313]

No description provided.

[Youssef1313]

cc: @gewarren

I also met an include that seem to indicate affected APIs, but indicates that in the Details or Suggestion heading.

[gewarren]

The text should say "Not detectable via API analysis".

[gewarren]

Thanks for fixing up those lists!

[gewarren]

Suggested change

	<!-- TODO: Affected APIs? -->
	<!-- TODO: Affected APIs? -->

[Youssef1313]

@gewarren, I'll cleanup this TODOs. The issue now is what to put under #### Affected APIs? I'm not sure if it's "None" in all the cases where the heading is missing. It could be missing despite the existence of affected APIs. Do you know what should I do?

[gewarren]

@PriyaPurkayastha How important is it to list all affected APIs for the .NET Framework migration guide docs? I don't think we have the resources to go through each one and determine if there are any missing affected APIs.

[gewarren]

Priya and I spoke offline and agreed that at this point, we have to assume that the section was omitted because there are no affected APIs. So we should just put "Not detectable via API analysis".

[Youssef1313]

@gewarren I wrote a tool to add the missing Affected APIs comments. But it looks like it have some wrong results in certain cases. I'll point to one example of these now to know the correct action.

Here is the code I wrote:

using System;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Net;
using System.Text.RegularExpressions;

namespace AffectedAPIsFixer
{
    public static class Extensions
    {
        public static List<string> GetAffectedAPIsAsXrefs(this FileInfo file)
        {
            string[] fileLines = File.ReadAllLines(file.FullName);
            var xrefs = new List<string>();
            var startReading = false;
            foreach (string line in fileLines)
            {
                if (startReading)
                {
                    string xref = Regex.Match(line, @"<xref:(.+?)(\?.+){0,1}>").Groups[1].Value;
                    if (!string.IsNullOrWhiteSpace(xref))
                    {
                        xrefs.Add(xref);
                    }
                }
                if (line == "#### Affected APIs")
                {
                    startReading = true;
                }

            }
            return xrefs;
        }

        public static string GetCommentIdForXref(this string xref)
        {
            var client = new WebClient(); // WebClient isn't really recommended. Just using it for simplicity.
            string response = client.DownloadString($"https://xref.docs.microsoft.com/query?uid={xref}");
            return Regex.Match(response, @"""commentId"":""(.+?)""").Groups[1].Value;
        }
    }

    public static class Program
    {
        public static void Main()
        {
            FileInfo[] files = GetMarkdownFiles();
            foreach (FileInfo file in files)
            {
                List<string> xrefs = file.GetAffectedAPIsAsXrefs();
                if (xrefs.Count == 0) continue;
                string[] ids = xrefs.Select(xref => $"- `{xref.GetCommentIdForXref()}`").ToArray();
                string commentToAppend = @$"
<!--

#### Affected APIs

{string.Join(Environment.NewLine, ids)}

-->
";
                // File.ReadAllText and File.WriteAllText might not be the most suitable APIs here (streams could do better). But just for simplicity. Optimization isn't important for me here.
                File.WriteAllText(file.FullName, File.ReadAllText(file.FullName) + commentToAppend);
                Console.WriteLine($"Done: {file.Name}");
            }
            Console.WriteLine("Done.");
        }

        private static FileInfo[] GetMarkdownFiles()
        {
            return new DirectoryInfo(@"D:\dotnet\docs\docs\includes\migration-guide\runtime").GetFiles("*.md", SearchOption.AllDirectories);
        }


    }
}

[adegeo]

Any file in the winforms area should be reverted, I'm in the process of migrating these files to a new repo.

[Youssef1313]

Any file in the winforms area should be reverted, I'm in the process of migrating these files to a new repo.

@adegeo, even the formatting fix?
and if these will be moved to a new repo, will the analyzer that detects compatibility issues still work correctly?

[adegeo]

@Youssef1313 correct. I've actually already moved the content but I'm working on redirects and publishing stuff at the moment. I don't know anything about the analyzer.

[Youssef1313]

I believe we should make sure that the compatibility analyzer doesn't break.

@gewarren Do you know how the analyzer works, and where it searches for info?

[gewarren]

@gewarren Do you know how the analyzer works, and where it searches for info?

I don't, but would like to find out. I'm wondering if the apiport tool even checks these any more, and if we still need the commented section of affected APIs. It looks like the list of breaking changes hasn't been updated in a couple of years. @mairaw or @PriyaPurkayastha Can you enlighten us?

[PriyaPurkayastha]

@Lxiamail whose team owns APIPort tool so that she can address questions related to the tool and how it uses the information from the Affected APIs section in the breaking changes.

[Lxiamail]

@Lxiamail whose team owns APIPort tool so that she can address questions related to the tool and how it uses the information from the Affected APIs section in the breaking changes.

Yes, .NET portability analyzer (APIPort) uses DocId ( in the format like M:System.Data.Common.DbConnectionStringBuilder.#ctor`) to idenitfy each individual API. if my understand it correctly, change PR changes the format. @terrajobst does this change impact the other .NET API tools?

[gewarren]

if my understand it correctly, change PR changes the format. @terrajobst does this change impact the other .NET API tools?

No, this PR is not to change the format of the affected APIs section. Youssef's initial purpose of this PR was to fix the lists (e.g. https://docs.microsoft.com/en-us/dotnet/framework/migration-guide/runtime/4.5.2-4.8#affected-apis) that were broken by #19227.

Then he noticed that some of these files didn't have the commented-out section of affected APIs, so he wrote a tool to add that section, using the DocId for each API. But a question came up - is this still important to do?

Now another question has come up - if Andy moves the WinForms breaking changes files to another repository, does that break the apiport tool?

[Lxiamail]

haha @gewarren thanks for your clarification. That makes sense.

Now another question has come up - if Andy moves the WinForms breaking changes files to another repository, does that break the apiport tool?
Yes, moving APIs to a different repo will make the analyzer missing the information APIs unless we update the analyzer tool to look for the new repo as well. What's the new repo, what APIs will be moved to there?

[Youssef1313]

Yes, moving APIs to a different repo will make the analyzer missing the information APIs unless we update the analyzer tool to look for the new repo as well.

@Lxiamail, Thanks! this made things clear now.

In that case, I think we shouldn't exclude winforms changes. If the tool will be updated to the new repo, it should have the fixed version of the markdown files.

cc: @adegeo

[Youssef1313]

Marking as WIP because it still needs the "Not detectable via API analysis" for files that are missing "Affected APIs" heading.

@gewarren, Most of the breaking changes that are missing "Affected APIs" are because there are indeed no Affected APIs. But there can be few occurrences where the heading is missing while there are affected APIs. Are you okay if I give a quick look and try to spot as much as I can of the erroneous missing Affected APIs?

[gewarren]

@gewarren, Most of the breaking changes that are missing "Affected APIs" are because there are indeed no Affected APIs. But there can be few occurrences where the heading is missing while there are affected APIs. Are you okay if I give a quick look and try to spot as much as I can of the erroneous missing Affected APIs?

Yes, that's fine, thanks.

[adegeo]

I'm moving the framework windows forms files to a new repo, not breaking changes. I would think that breaking changes could be summarized in the new winforms repo, but we should keep all breaking changes here so there is a complete user experience for anyone who is upgrading any code from framework to net/core.

[Youssef1313]

@adegeo All the files I changed here are breaking changes includes

[Youssef1313]

@gewarren, I added Affected APIs section here, it didn't exist before. Can you review it?

[adegeo]

Indeed.. since github cuts off the file path, I only saw a folder named winforms and assumed it was in framework/winforms :)

[Lxiamail]

Yes, moving APIs to a different repo will make the analyzer missing the information APIs unless we update the analyzer tool to look for the new repo as well.

@Lxiamail, Thanks! this made things clear now.

In that case, I think we shouldn't exclude winforms changes. If the tool will be updated to the new repo, it should have the fixed version of the markdown files.

cc: @adegeo

What's the new repo url?

[adegeo]

@Lxiamail It's just for WPF and WinForms content. https://github.com/dotnet/docs-desktop/

[BillWagner]

@gewarren @adegeo Is this ready to merge? It looks like its ready, but I wanted to check.