Ignore patterns specified in excludes in docfx.json #21
Conversation
| if (file.IsRenamed() && matcher?.Match(file.PreviousFileName)?.HasMatches == true && !IsExtensionChangeOnly(file.PreviousFileName, file.FileName)) | ||
| { | ||
| if (!await RedirectionsVerifier.WriteResultsAsync(Console.Out, file.PreviousFileName)) | ||
| { | ||
| returnCode++; | ||
| } | ||
| } | ||
| else if (file.IsRemoved() && !files.Any(f => f.IsAdded() && IsExtensionChangeOnly(file.FileName, f.FileName))) | ||
| else if (file.IsRemoved() && matcher?.Match(file.FileName)?.HasMatches == true && !files.Any(f => f.IsAdded() && IsExtensionChangeOnly(file.FileName, f.FileName))) |
There was a problem hiding this comment.
Currently the code matcher?.Match(......) is duplicated, but I have another PR that introduced a local function IsRedirectableFile. which will avoid duplication (depends on which PR gets merged first, this will be fixed when conflicts in the other PR are fixed).
|
|
||
| namespace RedirectionVerifier | ||
| { | ||
| public static class DocfxConfigurationReader |
There was a problem hiding this comment.
I'll look into having a base abstract "ConfigurationReader" class that simplifies the multiple readers we currently have and extract common logic into it.
There was a problem hiding this comment.
I'd be happy to write that part, once these all get in. I created an issue to track it here #23
| private IEnumerable<string> GetExcludes() | ||
| // This method retrieves all the "exclude" arrays under each of the "content"s. | ||
| // This can have "false negatives", since we are excluding patterns that can be meant to apply only to cross-reference repositories. | ||
| // For example, https://github.com/dotnet/docs/blob/bc8e38479d9867c7ed4b308be596ee7642422754/docfx.json#L48-L69 | ||
| // The exclude patterns in the link above are meant only for dotnet/csharplang repository. But we're taking them into account. | ||
| // If a file in the docs repo happen to match one of these patterns, it won't require a redirection, while it should. | ||
| // TODO: Fix that. One way to fix that is to add a flag to docfx.json for "content"s that are published. | ||
| // Another way is to not require any changes in docfx.json, but try to somehow find which "content" should be taken. |
There was a problem hiding this comment.
No, let's not get all the excludes. Let's get only the one that is relevant to the repo in context. Use the src and match it on the repo name for the GitHub context object.
See https://github.com/dotnet/docs/blob/bc8e38479d9867c7ed4b308be596ee7642422754/docfx.json#L15, the src is "docs" -- only these things should be excluded. And if you cannot find any matching by src, there is nothing to exclude.
There was a problem hiding this comment.
@IEvangelist It's not always the case from what I see in AspNetCore.Docs repo.
There was a problem hiding this comment.
Either match the name, or ".". "." just means the root, that should work -- right?
There was a problem hiding this comment.
@IEvangelist Is there a guarantee that the "dest" should either be . or the repo name? I'm afraid if checking the repo name, e.g "docs" can cause problems in the private localized repositories that are probably mirrored from the original repo?
I think one good way to do that is to try treating "dest" as a relative directory and see if it exists or not. There should only be one match. What do you think?
There was a problem hiding this comment.
Do you mean "src"? The "dest" is the destination for static file gen, the src is the source directory to build from. What I'm suggesting is that if you're looking for excludes, you should only consider excludes that A) have a src that matches the repo name, or B) a src that is .
There was a problem hiding this comment.
@IEvangelist Actually, this repo doesn't have a "src" specified at all, which may default ., but it has excludes.
There was a problem hiding this comment.
Yes, if there is no src assume it's "." and use those excludes.
There was a problem hiding this comment.
@IEvangelist I think it's a bit more complicated. The docfx.json may not be in the root, which is an incorrect assumption I have, and there may be multiple docfx.json files. I'm going to look into fixing that.
There was a problem hiding this comment.
I agree, and you're not wrong -- but I just think that for the scope of this specific enhancement we're trying to boil the ocean. While there are lots of additional considerations and edge cases, the reality is that as of right now and for the immediate future, this will only be used by a select few repos. IMO, it's better to prioritize around standardization, and create issues for future consideration that capture these potential concerns.
As of now, we're reviewing this current PR - and I think will it's ok it could be improved by what I was suggesting. Is that fair?
There was a problem hiding this comment.
@IEvangelist I think I could be near to get the more generalized approach working, but happy to switch back to the simpler approach, which as far as I understood works with like this:
- Always assume a single
docfx.jsonin repo root. - Exclude
contentthat hassrc==.orrepo-name(assuming the GH Action isn't run in the localized mirrors).
|
|
||
| namespace RedirectionVerifier | ||
| { | ||
| public static class DocfxConfigurationReader |
There was a problem hiding this comment.
I'd be happy to write that part, once these all get in. I created an issue to track it here #23
| { | ||
| Console.WriteLine("Adding matcher:"); | ||
| var matcher = new Matcher(StringComparison.OrdinalIgnoreCase); | ||
| // TODO: I'm not sure what "Exludes" and "Files" are relative to? |
There was a problem hiding this comment.
I believe that they are relative to src if one is present, otherwise relative to the root directory of the repo ..
|
@IEvangelist I'm going to close for now in favor of your suggested simpler approach: #25. |
No description provided.