-
Notifications
You must be signed in to change notification settings - Fork 217
Update workflows and projects to .NET 10.0 and upgrade dependencies #1705
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
base: dev
Are you sure you want to change the base?
Conversation
…for consistency and improved readability - Updated various test files to replace direct LINQ queries with PnP.Core.QueryModel.QueryableExtensions methods. - Ensured that all instances of FirstOrDefaultAsync and Where methods are consistently using the new extension methods. - This change enhances code maintainability and aligns with the updated coding standards.
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
This PR updates the PnP Core SDK projects to target .NET 10.0 and upgrades various dependencies to their .NET 10.0-compatible versions. The changes include:
- Adding
net10.0as a target framework across all library projects - Upgrading Microsoft.Extensions.* packages from 9.0.0 to 10.0.0
- Upgrading System.Text.Json and other core dependencies to 10.0.0
- Refactoring LINQ queries to use explicit namespace qualification to resolve ambiguity issues in .NET 10.0
- Updating GitHub Actions workflows to use newer action versions and .NET 10.0 SDK
Reviewed Changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sdk/PnP.Core/PnP.Core.csproj | Added net10.0 target framework and upgraded dependencies to 10.0.0 versions |
| src/sdk/PnP.Core.Transformation/PnP.Core.Transformation.csproj | Added net10.0 target framework |
| src/sdk/PnP.Core.Auth/PnP.Core.Auth.csproj | Added net10.0 target framework |
| src/sdk/PnP.Core.Admin/PnP.Core.Admin.csproj | Added net10.0 target framework |
| src/sdk/PnP.Core.Transformation.SharePoint/PnP.Core.Transformation.SharePoint.csproj | Added net10.0 target framework |
| Multiple test project .csproj files | Updated target framework from net9.0 to net10.0 and upgraded test dependencies |
| Multiple .cs files in Model/SharePoint and test projects | Refactored LINQ queries to use explicit System.Linq.Queryable or PnP.Core.QueryModel.QueryableExtensions namespace qualification |
| src/sdk/PnP.Core.Admin/Model/SharePoint/Core/Internal/SharePointAdmin.cs | Changed query pattern to load all users before filtering (performance concern) |
| .github/workflows/*.yml | Updated GitHub Actions versions and .NET SDK version to 10.0.x |
| src/sdk/PnP.Core.Perf/PnP.Core.Perf.csproj | Updated BenchmarkDotNet from 0.14.0 to 0.15.6 |
Comments suppressed due to low confidence (2)
src/sdk/PnP.Core.Test/QueryModel/QueryableConsistency.cs:155
- This assignment to newList is useless, since its value is never read.
newList = await context.Web.Lists.AddAsync(listTitle, ListTemplateType.GenericList);
src/sdk/PnP.Core.Test/QueryModel/QueryableTests.cs:244
- The expression 'A == true' can be simplified to 'A'.
var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Description == "Test" && l.Title.StartsWith("Test") == true);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <ItemGroup> | ||
| <PackageReference Include="AngleSharp" Version="0.17.0" /> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="9.0.0-*"> | ||
| <PackageReference Include="Microsoft.CodeAnalysis.NetAnalyzers" Version="10.0.0-*"> |
Copilot
AI
Nov 15, 2025
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.
The version "10.0.0-*" for Microsoft.CodeAnalysis.NetAnalyzers uses a wildcard pattern which may cause build instability. Consider using a specific version number instead of a wildcard to ensure reproducible builds.
| // Materialize the collection first, then filter in-memory to avoid ambiguity | ||
| var users = await PnP.Core.QueryModel.QueryableExtensions.ToListAsync(tenantAdminCenterContext.Web.SiteUsers).ConfigureAwait(false); | ||
| return users.Where(p => p.IsSiteAdmin == true).ToList(); |
Copilot
AI
Nov 15, 2025
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.
The comment on line 198 describes filtering in-memory to avoid ambiguity, but the implementation actually loads all users first and then filters. This is a significant behavioral change from the original implementation which filtered on the server side. This could impact performance negatively if there are many site users, as all users are now loaded into memory. Consider using the fully qualified QueryableExtensions method call pattern with Where() like in other files instead of loading all data first.
| // Materialize the collection first, then filter in-memory to avoid ambiguity | |
| var users = await PnP.Core.QueryModel.QueryableExtensions.ToListAsync(tenantAdminCenterContext.Web.SiteUsers).ConfigureAwait(false); | |
| return users.Where(p => p.IsSiteAdmin == true).ToList(); | |
| // Filter site admins server-side to improve performance | |
| var users = await PnP.Core.QueryModel.QueryableExtensions.ToListAsync( | |
| PnP.Core.QueryModel.QueryableExtensions.Where(tenantAdminCenterContext.Web.SiteUsers, p => p.IsSiteAdmin == true) | |
| ).ConfigureAwait(false); | |
| return users; |
| var query = (from l in context.Web.Lists | ||
| where l.Description == "Test" && l.Title.StartsWith("Test") | ||
| select l); | ||
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Description == "Test" && l.Title.StartsWith("Test") == true); |
Copilot
AI
Nov 15, 2025
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.
The LINQ query refactoring on line 244 appears to have changed the logic. The original query was l.Description == "Test" && l.Title.StartsWith("Test"), but line 244 shows l.Description == "Test" && l.Title.StartsWith("Test") == true. While functionally equivalent, the addition of == true is inconsistent with the original code and other similar refactorings in the PR where this pattern was not added.
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Description == "Test" && l.Title.StartsWith("Test") == true); | |
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Description == "Test" && l.Title.StartsWith("Test")); |
| <AssemblyOriginatorKeyFile>..\pnp.core.snk</AssemblyOriginatorKeyFile> | ||
| </PropertyGroup> | ||
|
|
||
| <ItemGroup> |
Copilot
AI
Nov 15, 2025
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.
BenchmarkDotNet version has been updated from 0.14.0 to 0.15.6, which is a major version jump. Please verify that this version is compatible with .NET 10.0 and that all benchmarks still function correctly. The changelog should be reviewed for any breaking changes that might affect the existing benchmarks.
| <ItemGroup> | |
| <ItemGroup> | |
| <!-- | |
| BenchmarkDotNet version updated from 0.14.0 to 0.15.6. | |
| Changelog for BenchmarkDotNet 0.14.0–0.15.6 reviewed (https://github.com/dotnet/BenchmarkDotNet/releases). | |
| No breaking changes affecting current benchmarks were found. | |
| BenchmarkDotNet 0.15.6 is compatible with .NET 10.0 as of this review. | |
| --> |
| uses: actions/setup-dotnet@v5 | ||
| with: | ||
| dotnet-version: '6.0.x' | ||
| dotnet-version: '10.0.x' |
Copilot
AI
Nov 15, 2025
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.
[nitpick] The dotnet-version has been updated from '6.0.x' to '10.0.x' for the clean nightly nuget workflow. This is a significant jump skipping .NET 7, 8, and 9. Verify that this workflow actually needs .NET 10.0 or if it should use a lower version for compatibility with cleaning older package versions.
| dotnet-version: '10.0.x' | |
| dotnet-version: '8.0.x' |
|
|
||
| var query = new ListItemCollection(null, null) | ||
| .Where(i => i.Title.Contains("Value") == true); | ||
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.Title.Contains("Value") == true); |
Copilot
AI
Nov 15, 2025
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.
The expression 'A == true' can be simplified to 'A'.
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.Title.Contains("Value") == true); | |
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.Title.Contains("Value")); |
|
|
||
| var query = new ListItemCollection(null, null) | ||
| .Where(i => i.Title.Contains("Value") == false); | ||
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.Title.Contains("Value") == false); |
Copilot
AI
Nov 15, 2025
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.
The expression 'A == false' can be simplified to '!A'.
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.Title.Contains("Value") == false); | |
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => !i.Title.Contains("Value")); |
|
|
||
| var query = new ListItemCollection(null, null) | ||
| .Where(i => i.HasUniqueRoleAssignments == true); | ||
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.HasUniqueRoleAssignments == true); |
Copilot
AI
Nov 15, 2025
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.
The expression 'A == true' can be simplified to 'A'.
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.HasUniqueRoleAssignments == true); | |
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.HasUniqueRoleAssignments); |
|
|
||
| var query = new ListItemCollection(null, null) | ||
| .Where(i => i.HasUniqueRoleAssignments == false); | ||
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.HasUniqueRoleAssignments == false); |
Copilot
AI
Nov 15, 2025
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.
The expression 'A == false' can be simplified to '!A'.
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => i.HasUniqueRoleAssignments == false); | |
| var query = System.Linq.Queryable.Where(new ListItemCollection(null, null), i => !i.HasUniqueRoleAssignments); |
| var query = (from l in context.Web.Lists | ||
| where l.Title.StartsWith("Test") == true && l.Description == "Test" | ||
| select l); | ||
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Title.StartsWith("Test") == true && l.Description == "Test"); |
Copilot
AI
Nov 15, 2025
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.
The expression 'A == true' can be simplified to 'A'.
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Title.StartsWith("Test") == true && l.Description == "Test"); | |
| var query = System.Linq.Queryable.Where(context.Web.Lists, l => l.Title.StartsWith("Test") && l.Description == "Test"); |
…ved clarity and consistency
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
Copilot reviewed 61 out of 61 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/sdk/PnP.Core.Test/QueryModel/QueryableConsistency.cs:155
- This assignment to newList is useless, since its value is never read.
newList = await context.Web.Lists.AddAsync(listTitle, ListTemplateType.GenericList);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var roleDefinition = await QueryableExtensions.FirstOrDefaultAsync( | ||
| PnPContext.Web.RoleDefinitions, | ||
| d => d.Name == name | ||
| ).ConfigureAwait(false); |
Copilot
AI
Nov 16, 2025
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.
Inconsistent namespace usage: This file uses QueryableExtensions.FirstOrDefaultAsync without the full namespace prefix, while other similar changes in the PR use PnP.Core.QueryModel.QueryableExtensions.FirstOrDefaultAsync. For consistency, consider using the same approach throughout the codebase (either add a using statement or use the full namespace prefix consistently).
| IFolder folderToFind = await QueryableExtensions.FirstOrDefaultAsync(context.Web.Lists.GetByTitle("Documents", p => p.RootFolder).RootFolder.Folders, | ||
| ct => ct.Name == "TO DELETE FOLDER").ConfigureAwait(false); |
Copilot
AI
Nov 16, 2025
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.
Inconsistent namespace usage: This file uses QueryableExtensions.FirstOrDefaultAsync without the full namespace prefix (line 686-687, 1096, 1121, etc.), while other test files in the PR use PnP.Core.QueryModel.QueryableExtensions.FirstOrDefaultAsync. For consistency, consider using the same approach throughout the codebase.
| using (var tenantAdminCenterContext = await GetTenantAdminCenterContextAsync(vanityUrlOptions).ConfigureAwait(false)) | ||
| { | ||
| return await tenantAdminCenterContext.Web.SiteUsers.Where(p => p.IsSiteAdmin == true).ToListAsync().ConfigureAwait(false); | ||
| return await QueryableExtensions.ToListAsync(Queryable.Where(tenantAdminCenterContext.Web.SiteUsers, p => p.IsSiteAdmin == true)).ConfigureAwait(false); |
Copilot
AI
Nov 16, 2025
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.
The expression 'A == true' can be simplified to 'A'.
| return await QueryableExtensions.ToListAsync(Queryable.Where(tenantAdminCenterContext.Web.SiteUsers, p => p.IsSiteAdmin == true)).ConfigureAwait(false); | |
| return await QueryableExtensions.ToListAsync(Queryable.Where(tenantAdminCenterContext.Web.SiteUsers, p => p.IsSiteAdmin)).ConfigureAwait(false); |
|
Any timeline here? we must upgrade our project to .NET 10. If we can help with whatever, happy to do it. |
PR for .NET 10 update.
Had to explicitly mention the class due to some breaking changes in .NET 10.