Skip to content
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

Move common S.P.CoreLib code under libs #98947

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 26, 2024

No description provided.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Feb 26, 2024
@am11 am11 added area-Infrastructure community-contribution Indicates that the PR has been added by a community member and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Feb 26, 2024
@ghost
Copy link

ghost commented Feb 26, 2024

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: am11
Assignees: -
Labels:

area-Infrastructure, community-contribution

Milestone: -

@jkotas
Copy link
Member

jkotas commented Feb 27, 2024

I am wondering whether it is intentional for

public sealed override Assembly GetSatelliteAssembly(CultureInfo culture) { throw new PlatformNotSupportedException(); }
public sealed override Assembly GetSatelliteAssembly(CultureInfo culture, Version version) { throw new PlatformNotSupportedException(); }

to throw PNSE instead of calling InternalGetSatelliteAssembly method.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

an.SetPublicKey(GetPublicKey());
an.Flags = GetFlags() | AssemblyNameFlags.PublicKey;
an.Version = version ?? GetVersion();
RuntimeAssembly runtimeAssembly = (RuntimeAssembly)assembly;
Copy link
Member

Choose a reason for hiding this comment

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

It may look better to keep this as an instance method and do the RuntimeAssembly cast on this in the one or two places that needs it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've matched mono and nativeaot signature and used it in libraries. Other two do not require an instance method.

@am11 am11 force-pushed the feature/code-consolidation branch from 7277da5 to c3126c3 Compare February 27, 2024 05:55
@am11
Copy link
Member Author

am11 commented Feb 27, 2024

I am wondering whether it is intentional for

public sealed override Assembly GetSatelliteAssembly(CultureInfo culture) { throw new PlatformNotSupportedException(); }
public sealed override Assembly GetSatelliteAssembly(CultureInfo culture, Version version) { throw new PlatformNotSupportedException(); }

to throw PNSE instead of calling InternalGetSatelliteAssembly method.

It seems to be like that for a long time: dotnet/corert@e5892d4.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas jkotas merged commit bff39aa into dotnet:main Feb 27, 2024
166 of 172 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Mar 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants