Skip to content

Conversation

@mihtjel
Copy link
Contributor

@mihtjel mihtjel commented Jul 16, 2023

The existing version of XRefCollection's ReaderCreator attempts to add items to a collection while it is traversing the same collection using foreach, causing an AggregateException. This error can be seen by having a simple xrefmap.yml file (referred to by "xref" in docfx.json) containing two redirections:

### YamlMime:XRefMap
redirections:
- uidPrefix: DocFx.PrefixOne
  href: https://example.com/folder1/xrefmap.yml
- uidPrefix: DocFx.PrefixTwo
  href: https://example.com/folder2/xrefmap.yml

This pull request reverts to a previous form of the loop, allowing the collection to be changed while processing it. Additionally, it adds a couple of log statements to XRefMapDownloader, enabled via --logLevel verbose, to allow the user to see which XRefMaps are being read.

@mihtjel
Copy link
Contributor Author

mihtjel commented Jul 16, 2023

I'll take the time to read the license agreement; but since this PR in its core is a revert to a previous version of the code, I make no claims of copyright, and the project is free to use its content without my involvement.

Copy link
Contributor

@yufeih yufeih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. @mihtjel we need the CLA check pass to merge this PR.

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (7ab1ad6) 76.93% compared to head (b79f34d) 76.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8972      +/-   ##
==========================================
- Coverage   76.93%   76.93%   -0.01%     
==========================================
  Files         605      605              
  Lines       25054    25057       +3     
==========================================
+ Hits        19275    19277       +2     
- Misses       5779     5780       +1     
Impacted Files Coverage Δ
src/Docfx.Build.Engine/XRefMaps/XRefCollection.cs 72.58% <100.00%> (+0.44%) ⬆️
...c/Docfx.Build.Engine/XRefMaps/XRefMapDownloader.cs 83.11% <100.00%> (+0.45%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mihtjel
Copy link
Contributor Author

mihtjel commented Jul 17, 2023

@dotnet-policy-service agree

@yufeih yufeih merged commit 96a5522 into dotnet:main Jul 18, 2023
@yufeih
Copy link
Contributor

yufeih commented Jul 18, 2023

Thank you @mihtjel for fixing this!

@yufeih yufeih added the bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note label Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug-fix Makes the pull request to appear in "Bug Fixes" section of the next release note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants