Switch to storing annotations in a static field #2426
Conversation
mhutch
force-pushed
the
static-annotation-storage
branch
3 times, most recently
from
d81fa05
to
7ae4784
Compare
Although it's kind of icky to store instance data on a static
field, it is implemented in a robust manner that should prevent
any surprises, and the details are hidden from authors of CLIs
and authors of subsystems.
A subsystem reference is no longer needed when annotating the
CliSymbol objects in the grammar, which makes construction
of a pipleline much simpler.
The fluent grammar construction API now looks like this:
```csharp
new Option<bool>("--greeting").WithDescription("The greeting")
```
Eventually we will be able to use extension properties:
```csharp
new Option<bool>("--greeting") {
Description = "The greeting"
}
```
We still support the `IAnnotationProvider` model that allows
advanced CLI authors to lazily or dynamically provide annotations.
mhutch
force-pushed
the
static-annotation-storage
branch
from
7ae4784
to
662ec29
Compare
There was a problem hiding this comment.
This is great.
I have several comments for consideration, and some nits.
My vote is to merge this as soon as possible so I can accommodate it in some outstanding work (currently in a draft PR that needs rebasing and will be merge fun).
If you need me to re-approve after changes, ping me in teams or text so I can update your approval.
| { | ||
| class AnnotationStorage : IAnnotationProvider | ||
| { | ||
| record struct AnnotationKey(CliSymbol symbol, string annotationId); |
There was a problem hiding this comment.
nit
annotationId is used here for a parameter that is not of AnnotatonId type. Consider different naming, especially since the next method calls an AnnotationId instance an id.
There was a problem hiding this comment.
Good catch, this was existing code (renamed from DefaultAnnotationProvider), but it turns out it was ignore the Prefix qualification. I restructured it to be clearer.
|
|
||
| namespace System.CommandLine.Subsystems.Annotations; | ||
|
|
||
| partial class AnnotationStorageExtensions |
There was a problem hiding this comment.
nit
Extensions is used in a type name that is not an extension or a static class for extension methods. It is probable that the Extension suffix will be widely used for extension types in C# 13.
There was a problem hiding this comment.
It is a static class for extension methods. It's just a partial class so the nested class can be split into a separate file.
| // | ||
| // This is fine, as we will have the following well-defined threading behavior: an annotated grammar and pipeline may | ||
| // only be constructed/modified from a single thread. Once the grammar/pipeline instance is fully constructed, it may | ||
| // be safely used from multiple threads. |
There was a problem hiding this comment.
We might want to add that once constructed, it is not safe to modify the grammar if it is in use.
| /// <summary> | ||
| /// Handles storage of annotations associated with <see cref="CliSymbol"/> instances. | ||
| /// </summary> | ||
| public static partial class AnnotationStorageExtensions |
There was a problem hiding this comment.
Does this class need to be public?
There was a problem hiding this comment.
Yes. Implementors of subsystems need to use it to store any newly defined annotations. However, it's in the System.CommandLine.Subsystems.Annotations so it's not something most users will see.
| /// provider take precedence over those stored in internal annotation storage. | ||
| /// </param> | ||
| /// <returns>The annotation value, if successful, otherwise <c>default</c></returns> | ||
| public static TValue? GetAnnotationOrDefault<TValue>(this CliSymbol symbol, AnnotationId<TValue> annotationId, IAnnotationProvider? provider = null) |
There was a problem hiding this comment.
I am a bit worried about this name as default has additional meaning in System.CommandLine
There was a problem hiding this comment.
Yes, but I'm not sure of a better name. This naming pattern is used by LINQ for SingleorDefault etc.
src/System.CommandLine.Subsystems/Subsystems/Annotations/AnnotationStorageExtensions.cs
Outdated
Show resolved
Hide resolved
| /// <param name="id">The id for the value to be retrieved. For example, an annotation ID for help is description</param> | ||
| /// <param name="id"> | ||
| /// The identifier for the annotation value to be retrieved. | ||
| /// For example, the annotation identifier for the help description is <see cref="HelpAnnotations.Description">. |
There was a problem hiding this comment.
Is the type name HelpAnnotation used by the CLI author? IOW, do CLI authors ever have to type HelpAnnotations.Description? If they do, I think we should consider simplifying the name.
There was a problem hiding this comment.
It's used IAnnotationProvider authors and subsystem authors. What would you simplify it to?
| protected internal void SetAnnotation<TValue>(CliSymbol symbol, AnnotationId<Func<TValue>> id, Func<TValue> value) | ||
| { | ||
| (_defaultProvider ??= new DefaultAnnotationProvider()).Set<Func<TValue>>(symbol, id, value); | ||
| return symbol.TryGetAnnotation(id, out value, _annotationProvider); |
There was a problem hiding this comment.
I'm not sure we still need this method. As a convenience, perhaps wait until we understand the calling code better.
There was a problem hiding this comment.
I will revise the comment. It's not just a convenience, it's the way that subsystem authors must access annotations if they want to respect the subsystem's IAnnotationProvider, as the provider is private.
mhutch
force-pushed
the
static-annotation-storage
branch
from
802b5b2
to
b28ce92
Compare
Although it's kind of icky to store instance data on a static field, it is implemented in a robust manner that should prevent any surprises, and the details are hidden from authors of CLIs and authors of subsystems.
A subsystem reference is no longer needed when annotating the CliSymbol objects in the grammar, which makes construction of a pipleline much simpler.
The fluent grammar construction API now looks like this:
Eventually we will be able to use extension properties:
Note that we still support the
IAnnotationProvidermodel that allows advanced CLI authors to lazily or dynamically provide annotations.I'm still trying to figure out ways to enforce type safety when using
DefaultValue/DefaultValueCalculationwith the underlying annotation storage methodsSetAnnotation/TryGetAnnotation. However, theSetDefaultValueandSetDefaultValueCalculationspecialized setters will at least ensure safety for CLI authors.