-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Fix SDK-style (modern) inter-project references #3502
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
Fix SDK-style (modern) inter-project references #3502
Conversation
| } | ||
|
|
||
| if (!projects.Any()) | ||
| var projectList = projects.ToList(); |
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.
I am not exactly sure why the parameter is using IEnumerable here, I think it would make sense to just change the parameter type and/or add an overload, while we're at it. This change is going into the next major release, so breaking changes are allowed.
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.
I'm happy to just change it to take a List<> if breaking changes are fine. An API purist might suggest IReadOnlyCollection<> but here I slightly prefer giving the compiler all the help it can get devirtualizing (in software like this where perf is very nice).
(Incidentally, the "IEnumerable<> API which is .ToList() immediately" pattern is less bad than it might seem at first glance -- .ToList() won't reallocate if the IEnumerable<> reference represents a List<> already (at least, that's how it worked last time I checked).)
|
I haven't tested these two changes yet, but thought I'd throw them up here so you can see. |
bb56b80 to
848db48
Compare
848db48 to
0959f4d
Compare
|
This version isn't right; sdk-style projects shouldn't be writing |
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.
Pull Request Overview
Fixes XML namespace handling for SDK-style solution files, streamlines IEnumerable usage, and standardizes GUID casing.
- Consolidated repeated enumeration of
projectsby materializing to a list forWriteSolutionFile. - Added SDK-style detection to apply correct XML namespaces when fixing project references.
- Renamed the namespace constant and updated project GUID elements to lowercase.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ILSpy/Util/SettingsService.cs | Cleaned up closure syntax and removed a redundant standalone semicolon. |
| ILSpy/SolutionWriter.cs | Converted projects enumerable to List<ProjectItem> for WriteSolutionFile call. |
| ICSharpCode.Decompiler/Solution/SolutionCreator.cs | Renamed ProjectFileNamespace, changed several method signatures to use List<ProjectItem>, added SDK-style detection logic, and updated project reference fixing. |
Comments suppressed due to low confidence (3)
ILSpy/SolutionWriter.cs:156
- Forcing callers to call
.ToList()tightens the API contract. Consider restoring the originalIEnumerable<ProjectItem>parameter inWriteSolutionFileor providing an overload to avoid extra allocations.
await Task.Run(() => SolutionCreator.WriteSolutionFile(solutionFilePath, projects.ToList()))
ICSharpCode.Decompiler/Solution/SolutionCreator.cs:44
- Changing this public method to accept
List<ProjectItem>instead ofIEnumerable<ProjectItem>is a breaking change. To maintain compatibility, consider usingIEnumerable<ProjectItem>or adding an overload.
public static void WriteSolutionFile(string targetFile, List<ProjectItem> projects)
ICSharpCode.Decompiler/Solution/SolutionCreator.cs:176
- The conditional mixes
stringandXName, causing a type mismatch. Both branches should returnXName, for example:XName itemGroupTagName = sdkStyle ? XName.Get("ItemGroup") : NonSDKProjectFileNamespace + "ItemGroup";.
var itemGroupTagName = sdkStyle ? "ItemGroup" : NonSDKProjectFileNamespace + "ItemGroup";
…File(). (This is potentially expensive and the method is public, just a minor code smell.)
I accept any sideways glance for the allocation-averse code
9788e44 to
3862546
Compare
This comment was marked as outdated.
This comment was marked as outdated.
3862546 to
de73249
Compare
|
... aaand it looks like my lf vs crlf fixes aren't quite right (I'm using an SCM that doesn't respect git's autocrlf setting). Apologies, fixing. |
de73249 to
393b848
Compare
|
The pull request was misnamed -- it does affect ilspycmd (when decompiling to a solution, that is, using --project/-p with more than 1 assembly), but it changes ICSharpCode.Decompiler/Solution/SolutionCreator.cs, which is also called by the ILSpy GUI. PR SummaryFixes the Breaking Change
Enhancement
Fixes
|
393b848 to
f32c93f
Compare
f32c93f to
fdb0703
Compare
|
They say 7th force push is the charm, right? Ok, finally correctly removed the spurious change to SettingsService.cs, the diffs in github.com look good, and the build actions in my fork run cleanly. Everything should be good for review. |
|
Thank you very much for your contribution! |
Link to issue(s) this covers
Problem
The XML namespace isn't correctly configured when using new-style solution files.
Solution
There are three changes in this PR; I bundled them here as they are near in code and I didn't think anyone would object to the cleanup. (Please let me know if you'd rather I split up the changes.)