IconUrl deprecation message. #7556
Conversation
agr
requested review from
joelverhagen,
loic-sharma,
xavierdecoster,
shishirx34,
ryuyu,
cristinamanum,
skofman1,
dannyjdev,
scottbommarito and
zhhyu
| namespace NuGetGallery | ||
| { | ||
| /// <summary> | ||
| /// IconUrl element deprecation message. |
There was a problem hiding this comment.
nit: Not a very useful comment, as it just restates the name of the class. I would prefer either getting rid of this comment or saying more about IconUrl and its deprecation.
| Assert.Null(result.Message); | ||
| var warning = Assert.Single(result.Warnings); | ||
| Assert.IsType<IconUrlDeprecationValidationMessage>(warning); | ||
| Assert.StartsWith("<iconUrl> element will be deprecated, please consider switching to specifying the icon in the package.", warning.PlainTextMessage); |
There was a problem hiding this comment.
It looks easy to break this test if we use the hard coded string. Maybe directly using the "Strings.UploadPackage_IconUrlDeprecated" is better for future maintenances.
I find some other examples in this facts.
There was a problem hiding this comment.
That's the point of this check. To break if text changes. Using Strings.UploadPackage_IconUrlDeprecated directly will hide such changes.
There was a problem hiding this comment.
Like that?
Saying that element is deprecated when we didn't release a "stable" version of the client supporting it sounds a bit weird though.
@agr Do you need to update your tests?
|
Can I see a mock with all possible warnings? I am afraid we may have one link for every warning and then another read more at the end which might be few too many links. We can choose to not fix this right now in which case the warning message looks fine. I'd just make sure the language aligns with what I'd see on nuget pack. @dominoFire thoughts on the warning message? |
|
@karann-msft , this is the second warning message with the link (the other one is about license URL deprecation). All other warnings don't have links. And all other messages with link are errors (which we don't display at the same time as warnings). |
src/NuGetGallery/Strings.resx
Outdated
| @@ -1150,4 +1150,7 @@ The {1} Team</value> | |||
| <data name="SiteAdminNotLoggedInWithRequiredTenant" xml:space="preserve"> | |||
| <value>The site admins are required to sign in with the '{0}' tenant only.</value> | |||
| </data> | |||
| <data name="UploadPackage_IconUrlDeprecated" xml:space="preserve"> | |||
| <value><iconUrl> element will be deprecated, please consider switching to specifying the icon in the package.</value> | |||
There was a problem hiding this comment.
Considering that we're warning on icon URLs already, isn't it already deprecated? What do you think of this messaging?
| <value><iconUrl> element will be deprecated, please consider switching to specifying the icon in the package.</value> | |
| <value>The <iconUrl> element is deprecated, please use the icon element instead.</value> |
This is message doesn't mirror the client's message, but I would prefer a clearer message if possible.
There was a problem hiding this comment.
The message that client currently produces is:
WARNING: NU5048: The 'PackageIconUrl'/'iconUrl' element is deprecated. Consider using the 'PackageIcon'/'icon' element instead. Learn more at https://aka.ms/deprecateIconUrl
And for licenseUrl:
WARNING: NU5125: The 'licenseUrl' element will be deprecated. Consider using the 'license' element instead.
|
Let's update both messages to say "is deprecated". Rest looks good for now. |
|
Like that? Saying that element is deprecated when we didn't release a "stable" version of the client supporting it sounds a bit weird though. |
Does the client in .NET Core 3 warn? If so, that ships on the 23rd |
Addresses #7478
Browser:
Command line:
