Metadata mapping fixes/improvements #95
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
gpeairs
force-pushed
the
gp/fix-map-meta
branch
from
0af67be to
4561dba
Compare
gpeairs
force-pushed
the
gp/fix-map-meta
branch
from
4561dba to
6dd165d
Compare
parrangoiz
left a comment
parrangoiz
left a comment
There was a problem hiding this comment.
Looks good, just one minor comment.
src/schematics/targets.jl
Outdated
| map_layer(::Target, m::GDSMeta) = m # No need to warn | ||
|
|
||
| # For GDSMeta, return as-is (pass through) | ||
| _map_layer(target, meta::GDSMeta) = meta |
There was a problem hiding this comment.
Why the change of signature?
There was a problem hiding this comment.
I try to default to not using type annotations when they're not necessary for dispatch or correctness. In this case there's not much risk of calling it with a "wrong" type and getting an answer anyway. However I didn't actually think about it that much here. Mainly I removed ::Target to match the other internal _map_layer method below it, but I don't think it matters either way.
There was a problem hiding this comment.
I see why it's convenient to drop the type when it's meant to be a fallback, but in this case why not just keep it? Seems to me that we're just creating chances for future failures, for example if more _map_layer methods are defined, and then someone intending to call those methods accidentally ends up calling this one because of the more relaxed signature.
There was a problem hiding this comment.
Fair enough, the SchematicDrivenLayout namespace is pretty big and this is a lighter way to protect against that than a submodule. I've added the annotation to both _map_layer methods.
|
All good to merge on my end. |
2 participants
This includes four related fixes giving users more control over what layers are rendered, especially in SolidModelTarget:
NORENDER_META(the:norenderlayer) (closes SolidModelTarget should not render :norender layer #89)ignored_layers, a list of layer symbols which are not renderedGDSMetaby settingtarget.map_meta_dict[my_gdsmeta] = my_override, allowing changes to differentGDSMetaornothingrather than always mapping aGDSMetato itselfremove_group!SolidModel postrendering operation to useremove_entities=trueby default, fixing the unexpected and undesired default behavior that only removed the record of the group and not its entities (grouped with these fixes because this is how users were previously 'ignoring' layers in SolidModel) (closes Postrender operation remove_group! doesn't remove entities by default #90)