bebop / poly

A Go package for engineering organisms.
https://pkg.go.dev/github.com/bebop/poly
MIT License
663 stars 70 forks source link

`Gff.AddFeature()` code is misleading and mutates `Feature` state #342

Open carreter opened 11 months ago

carreter commented 11 months ago

Gff.AddFeature() seems to intend to create a copy of the Feature it is provided. Dereferencing the pointer is not sufficient to do this, as Feature structs contain mutable fields (e.g. Feature.Attribtues is a map[string]string, and Feature.Location is a struct that has a slice field in it).

https://github.com/TimothyStiles/poly/blob/ad662c741528bbb5bcaa7a3107991a86426c1e55/io/gff/gff.go#L78-L84

IMO, the Feature.ParentSequence field should be dropped entirely. Currently, it is only used by getFeatureSequence, which could easily be modified to be a method of the Gff struct. This makes more sense as Features and Locations only exist in the context of a Gff anyway.

This also means that the Gff.AddFeature() method can be entirely dropped in favor of directly appending it to Gff.Features.

It's a small change, so I'm more than happy to do this once given the go-ahead!

Blocked by #339 .

Koeng101 commented 11 months ago

I think I talked to @TimothyStiles about this one, and basically - yea its both a problem here AND in genbank. It'd probably be easiest to fix from #339 branch, so the merge will be easier. 100% we should make this better!

carreter commented 11 months ago

I'll wait until you merge #339 then do this.

github-actions[bot] commented 11 months ago

Status: Ready to merge :heavy_check_mark:

Issues blocking this PR:

github-actions[bot] commented 8 months ago

This issue has had no activity in the past 2 months. Marking as stale.

github-actions[bot] commented 6 months ago

This issue has had no activity in the past 2 months. Marking as stale.