Create FlaggedType as a reusable replacement for StorageType and ArmType #331
Conversation
theunrepentantgeek
requested review from
babbageclunk,
devigned,
matthchr and
Porges
as code owners
theunrepentantgeek
force-pushed
the
feature/flagged-types
branch
from
aec572a
to
2f0d314
Compare
| } | ||
|
|
||
| // Wrap() applies the tag to the provided type | ||
| func (f TypeFlag) Wrap(t Type) *FlaggedType { |
There was a problem hiding this comment.
minor: Normally when I think flags I don't think they "Wrap" things. Wrapping here is sortof an implementation detail of our type hierarchy, wondering if Apply (or ApplyTo?) would be better?
There was a problem hiding this comment.
Renamed to ApplyTo() as suggested.
There was a problem hiding this comment.
(Wrap() was a temporary name, the best I could think of at the time; happy to have a better suggestion.)
There was a problem hiding this comment.
Yeah I'm not married to Apply/ApplyTo either if others have better suggestions.
| t.Run(c.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| g := NewGomegaWithT(t) | ||
| ot, err := TypeAsObjectType(c.subject) |
There was a problem hiding this comment.
I'm not sure that this is what we want actually.
Right now practically speaking the TypeAsObjectType function only collapses ARM types. I know there's code in there to collapse Optional types too but even that I think is actually not correct (in the context of its use). I don't think the Optional path is taken right now because this is always called on a definition and definitions aren't ever to optional types (only properties are).
The problem I see here is that collapsing all flags when this is called means that no flag can persist past the ARM type conversion phase (or any phase that makes use of this function).
I think we want to allow preservation of flags all the way until the time we're writing code to disk (maybe even past that if we have post-writing phases?)...
It's late on a Friday and I'm maybe a bit crazy but is the problem that we have this desire to do stuff to Object (examine it, modify it, etc) and it's difficult to do that when it's wrapped? Maybe we need to rewrite the ARM to use a visitor so that it can preserve the wrapping types? (I think I wrote it originally before Visitor was added and then thought a bit about refactoring it but it melted my brain at the time).
Or maybe we need to write some psudeo-monadic functions on FlagType that can do "stuff" with the inner type but all without leaving the context of the wrapper? (@Porges)
It's possible you fix this in a future PR and if so feel free to ignore this
There was a problem hiding this comment.
I think TypeAsObjectType() is correct for our current usage, but I also see where you're coming from. I'll skip doing this here, but will lodge an issue (#335) so we don't forget to circle back and eliminate it.
There was a problem hiding this comment.
This seems an ok course of action. Will leave this active though as I am curious about @Porges and @babbageclunk's thoughts on a pattern that can achieve what this function wants to achieve but in a way that is more amenable to "other types"
theunrepentantgeek
force-pushed
the
feature/flagged-types
branch
from
2f0d314
to
084549b
Compare
theunrepentantgeek
force-pushed
the
feature/flagged-types
branch
from
084549b
to
60b4feb
Compare
| const ( | ||
| StorageFlag = TypeFlag("storage") | ||
| ArmFlag = TypeFlag("arm") | ||
| OneOfFlag = TypeFlag("oneof") |
There was a problem hiding this comment.
I'll rebase onto this (or wait until it's merged depending on how quickly it merges) and use this to resolve the open issue in PR #309
|
|
||
| var _ fmt.Stringer = TypeFlag("") | ||
|
|
||
| // String() renders the tag as a string |
There was a problem hiding this comment.
minor: Missed some () on these method comments
There was a problem hiding this comment.
Fixed.
I've had this as a habit, so may well have missed others - will annihilate as discovered.
| t.Run(c.name, func(t *testing.T) { | ||
| t.Parallel() | ||
| g := NewGomegaWithT(t) | ||
| ot, err := TypeAsObjectType(c.subject) |
There was a problem hiding this comment.
This seems an ok course of action. Will leave this active though as I am curious about @Porges and @babbageclunk's thoughts on a pattern that can achieve what this function wants to achieve but in a way that is more amenable to "other types"
Co-authored-by: Matthew Christopher <matthchr@microsoft.com>
…infra into feature/flagged-types
Create new
TypeimplementationFlaggedTypethat can be a wrapper around any existingType.1st of 3 related PRs.