Skip to content

Razor EA: load from ServiceHubCore #77720

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Mar 31, 2025
Merged

Razor EA: load from ServiceHubCore #77720

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d32aa53
Razor EA: load from ServiceHubCore
chsienki Mar 20, 2025
45c187f
Update src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs
chsienki Mar 21, 2025
36c0c51
Trim trailing slashes
chsienki Mar 24, 2025
a1e46a5
Update src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs
chsienki Mar 25, 2025
23bd4b1
Fix trim call
chsienki Mar 25, 2025
259a898
PR feedback
jaredpar Mar 31, 2025
97784b3
Merge remote-tracking branch 'origin/main' into fix_razor_loading
jaredpar Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Tools/ExternalAccess/Razor/RazorAnalyzerAssemblyResolver.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ internal sealed class RazorAnalyzerAssemblyResolver() : IAnalyzerAssemblyResolve
// or the ServiceHubCore subfolder (razor). In the root directory these are netstandard2.0 targeted, in ServiceHubCore they are
// .net targeted. We need to always pick the same set of assemblies regardless of who causes us to load. Because this code only
// runs in a .net based host, it's safe to always choose the .net targeted ServiceHubCore versions.
if (!directory.AsSpan().TrimEnd('/', '\\').EndsWith(ServiceHubCoreFolderName, StringComparison.OrdinalIgnoreCase))
if (!directory.AsSpan().TrimEnd("/\\").EndsWith(ServiceHubCoreFolderName, StringComparison.OrdinalIgnoreCase))
Copy link
Member

Choose a reason for hiding this comment

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

Oh no, please no. Use the separate invocation form.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not equivalent though, is it?

var x = "abc/\\/";
x.TrimEnd("/\\")              // "abc"
x.TrimEnd("/").TrimEnd("\\")  // "abc/"
x.TrimEnd("\\").TrimEnd("/")  // "abc/\"

Copy link
Member

Choose a reason for hiding this comment

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

There is no consistency in what the string overload here means, which is why string.Trim(ROS) was pulled from 9 in RC2. I don't know where this string-accepting extension is defined, but it's ambiguous whether it means trim exactly the string provided, or trim any of the characters provided. In any case, a collection expression of both items should work, and should be non-ambiguous as to the meaning.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure what your suggestion is here @333fred. Are you saying it should use a collection expr for the characters?

Copy link
Member

Choose a reason for hiding this comment

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

Are you saying it should use a collection expr for the characters?

Correct. Admittedly, part of this is just that the string version is making me queasy given the previous history around this method in .NET 9, but I do think we should opt for clear semantics, not ambiguous ones.

Copy link
Member

Choose a reason for hiding this comment

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

@333fred responded to feedback

Copy link
Member

Choose a reason for hiding this comment

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

Even better solution.

{
directory = Path.Combine(directory, ServiceHubCoreFolderName);
}
Expand Down
Loading