-
Notifications
You must be signed in to change notification settings - Fork 9
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
Assembly tagging #17
base: master
Are you sure you want to change the base?
Assembly tagging #17
Conversation
…accumulation test
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.
Let's talk over these comments before continuing.
@@ -23,7 +23,7 @@ type PragmaArgShape = | |||
|
|||
type PragmaValidationResult = Result<unit,string> | |||
|
|||
type PragmaPersistence = | Persistent | Transient | |||
type PragmaPersistence = | Persistent | Transient | TransientCumulative |
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.
I'm concerned that this is now mixing two orthogonal concerns; whether or not a pragma is transient or not is orthogonal to whether or not a pragma should accumulate arguments when multiple instances of it appear.
Probably best thing to do here is refactor this to remove the additional variant from PragmaPersistence and add a field on PragmaDef, perhaps accumulateArgs: bool
. That should simplify the implementation of Add below.
@@ -0,0 +1,90 @@ | |||
/// Assembly transforming plugin that implements seamless part assembly. |
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.
I not sure if a plugin is appropriate for implementing this feature. We had to extend the static type signature of assembly to accommodate this feature, so it seems to me like we should integrate the tagging addition directly into the assembly transformation stage. There is already a list of default assembly transformations that always happen and I think this is appropriate to add there.
Hopefully this one is a little simpler than last PR. This adds the
#tag namespace:tag
feature and extends pragma semantics to add a transient cumulative option.