Add support for DisplayId and UrlFormat - #69
Conversation
|
Should there also be a diagnostic for having a |
Agreed. I'll add |
|
@DavidBoike pushed |
There was a problem hiding this comment.
Can I actually suggest that all the URL formats be changed:
- From
https://docs.particular.net/obsoletions/{0} - To:
https://docs.particular.net/r/obsoletions/{0}
Because if I were building this into Docs today, I would use the /r/ path as a prefix that I know nothing else would ever use naturally, where r stands for redirect, and then that would allow /r/obsoletions/{0} to be joined by /r/exceptions/{0} or other redirect types that could be defined in a YAML config rather than additional code.
Sure it was more of a "fake look alike value" but that's fine too |
Co-authored-by: David Boike <david.boike@gmail.com>
|
|
||
| if (actualDiagnosticId != expectedDiagnosticId) | ||
| { | ||
| var diagnosticIdArgument = GetAttributeArgumentSyntax(obsoleteAttributeArguments, "DiagnosticId"); |
There was a problem hiding this comment.
I'm wondering if we should split this out into more cases instead of using the single diagnostic for all of them.
For example, for error and message, we have a distinction between "ObsoleteAttributeMissingConstructorArguments" diagnostic and the "Incorrect" diagnostics.
| /// </para> | ||
| /// </remarks> | ||
| /// <seealso cref="DiagnosticId" /> | ||
| public string? UrlFormat { get; set; } |
There was a problem hiding this comment.
With David bringing up the idea of having the standard URL format (https://docs.particular.net/r/obsoletions/{0})
I'm thinking we might want to bake this in instead of just directly exposing the value.
Maybe we just provide the DiagnosticId, and then we generate the expected UrlFormat property from that.
So, if neither is provided, neither of them are in the resulting Obsolete.
If DiagosticId is provided, we use it and then have a standard UrlFormat of https://docs.particular.net/r/obsoletions/{0} generated.
Are we thinking we'd want to have other URLs be options, like linking to a GitHub issue or something? If so, then maybe we just have a separate "URL" property for that instead, and maybe it can't have a placeholder?
There was a problem hiding this comment.
Yeah we need other URLs for the internal obsoletion cases so that we can link to a GitHub issue. I'm ok making it more restrictive. Originally I thought because the infrastructure is not yet there it is better to mimic the original attribute but as long as we allow a simple non templated URL it's fine and we get less diagnostic cases to deal with
There was a problem hiding this comment.
There is an argument to be made though that currently the particular obsolete package is quite close to the obsolete attribute and it only enforces a convention about the message and the flag based on the versions. As soon as we enforce a fixed URL it is very focused on our use case. Given an URL can be a constant in code I wonder if it is really worth hard coding the URL. I guess the upside would be you change it in one place and it is correct in all repos.
I'll wait with further changes except the diagnostic suggestion so that we can align what's best
There was a problem hiding this comment.
Brandon and I talked about how maybe it's better to agree on not just a fixed URL convention but also a fixed prefix (i.e. PSO for Particular Software Obsolete) and then just have an integer ObsoleteId. That would prevent a "wild west" situation of prefixes and prevent problems with codes overlapping with Roslyn diagnostic ids. That led to a conversation of "well then what rations those codes out" and it got complex from there given the ramifications for a redirect service and the workflow around that. We agreed to think on it over the weekend.
There was a problem hiding this comment.
I’m not convinced that preventing a “wild west” of identifiers requires a single, globally incrementing number range across all components.
We already use component-specific conventions for diagnostic IDs, and that appears to work reasonably well, at least in the areas I’ve been involved with, such as the assembly-scanning task force. A component prefix provides global uniqueness, while the number can be allocated and managed within the component repository.
That keeps the coordination local. Each repository can centralize its identifiers so that it is clear which values are already assigned and which convention applies. If we need to distinguish diagnostics, obsoletions, and exceptions, we can do that through an explicit prefix or locally reserved ranges.
We may still need a small registry of component prefixes to prevent those from overlapping. That seems substantially simpler than maintaining and allocating numbers from one global sequence across every component.
I also think the identifier scheme and the URL convention are separate concerns. We can standardize a redirect URL around a globally unique identifier without requiring that identifier to be a PSO prefix followed by a centrally allocated integer.
The trade-off I see is between lightweight coordination when establishing a component prefix and ongoing central coordination whenever any component introduces an obsoletion. I currently favor the former.
There was a problem hiding this comment.
But then, it has to be a unique prefix from the Roslyn prefix. No NSB.
What
Adds
DiagnosticIdandUrlFormatas optional properties onObsoleteMetadataAttribute. The analyzer and code fix provider automatically propagate them to the generated[Obsolete]attribute.Why
Since .NET 5,
ObsoleteAttributesupportsDiagnosticIdandUrlFormatfor stable suppression IDs and clickable help links in IDE tooltips and build output. We had to work around it by manually adding them to the[Obsolete]attribute, outside the metadata-driven validation and code fixing the package provides.What it enables
DiagnosticIdandUrlFormatonObsoleteMetadata, and the fixer generates them on the[Obsolete]attribute automaticallyDiagnosticIdfollows the convention of a short repository prefix followed by a zero-padded number (e.g.NSB0001), so consumers can suppress warnings with#pragma warning disable NSB0001or<NoWarn>instead of the genericCS0618UrlFormatsupports two patterns:{0}— the compiler substitutes the diagnostic ID, useful when the URL slug matches the ID (e.g.https://docs.particular.net/obsoletions/{0}){0}— used as-is, useful for direct links to specific resources like GitHub issues (e.g.https://github.com/Particular/NServiceBus/issues/42)Validation
Two new diagnostics catch metadata mistakes that would otherwise fail silently:
OBSOLETES0014—DiagnosticIdis empty or contains whitespace, which breaks#pragma/NoWarnsuppressionOBSOLETES0015—UrlFormatcontains more than one{0}placeholder, because the compiler silently ignores the entire URL in that caseWhat's missing
We don't yet have support for
DiagnosticIdandUrlFormatusing a placeholder in the docs engine. The template URL pattern (https://docs.particular.net/obsoletions/{0}) needs a docs page per diagnostic ID, which doesn't exist yet. I'll raise that as a separate feature request so the docs engine can serve those pages.