Skip to content
This repository was archived by the owner on Jul 29, 2025. It is now read-only.

Comments

Assembly tagging#17

Open
daz10000 wants to merge 8 commits intoAmyris:masterfrom
demetrixbio:assembly_tagging
Open

Assembly tagging#17
daz10000 wants to merge 8 commits intoAmyris:masterfrom
demetrixbio:assembly_tagging

Conversation

@daz10000
Copy link
Contributor

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.

Copy link
Contributor

@chrismacklin chrismacklin left a 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.

type PragmaValidationResult = Result<unit,string>

type PragmaPersistence = | Persistent | Transient
type PragmaPersistence = | Persistent | Transient | TransientCumulative
Copy link
Contributor

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.
Copy link
Contributor

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants