-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Make ResourceManager.BaseName work as documented. #75497
Conversation
This property is documented to return the "qualified namespace name and the root resource name of a resource file". However, previously when constructing a ResourceManager with a Type the BaseName property would instead return the name of the type without its namespace. fix dotnet#74918
Tagging subscribers to this area: @dotnet/area-system-resources Issue DetailsThis property is documented to return the "qualified namespace name and the root resource name of a resource file". However, previously when constructing a ResourceManager with a Type the BaseName property would instead return the name of the type without its namespace. fix #74918
|
src/libraries/System.Private.CoreLib/src/System/Resources/ResourceManager.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Resources/FileBasedResourceGroveler.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Resources/ManifestBasedResourceGroveler.cs
Show resolved
Hide resolved
src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs
Outdated
Show resolved
Hide resolved
Sorry for taking a while to look at this, thanks for the contribution! Could you also please add more tests that ensure the right behavior of BaseName? For example, can you add one where the type doesn't have a namespace too? |
Address feedback from dotnet#75497 (comment).
Assert.Equal(typeWithoutNamespace.Name, manager.BaseName); | ||
|
||
manager = new ResourceManager(typeof(List<string>)); | ||
Assert.Equal("System.Collections.Generic.List`1", manager.BaseName); | ||
} |
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.
@joperezr any other test cases you'd like to see?
src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs
Show resolved
Hide resolved
Thanks for the fix @madelson, but it might be not a bug needs a fix, see my comment on the issue for details, we might need to clarify the doc instead, I will dig into it more Also there is existing test that specifically checking this scenario is failing on CI
|
Thanks for engaging @buyaa-n . I'll fix that test if we decide to proceed with this change. |
Please do that so that we can see if any other test fails |
@buyaa-n pushed the fix! |
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.
Overall LGTM, thanks @madelson
This property is documented to return the "qualified namespace name and the root resource name of a resource file". However, previously when constructing a ResourceManager with a Type the BaseName property would instead return the name of the type without its namespace.
fix #74918