Skip to content
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

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

Open
carreter opened this issue Sep 11, 2023 · 5 comments
Open

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

carreter opened this issue Sep 11, 2023 · 5 comments
Labels
bug Something isn't working easy A quick and easy fix! good first issue Good for newcomers medium priority The default priority for a new issue. stale
Milestone

Comments

@carreter
Copy link
Collaborator

carreter commented Sep 11, 2023

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

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
Copy link
Collaborator Author

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

@carreter carreter added bug Something isn't working good first issue Good for newcomers easy A quick and easy fix! medium priority The default priority for a new issue. labels Sep 15, 2023
@github-actions
Copy link

github-actions bot commented Sep 23, 2023

Status: Ready to merge ✔️

Issues blocking this PR:


This comment was automatically written by the Blocking Issues bot, and this PR will be monitored for further progress.

@github-actions github-actions bot added the blocked Waiting for another PR/issue to be merged/closed. label Sep 23, 2023
@carreter carreter added this to the v1.0 milestone Sep 23, 2023
@abondrn abondrn mentioned this issue Oct 30, 2023
6 tasks
Copy link

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

@github-actions github-actions bot added the stale label Nov 22, 2023
@github-actions github-actions bot removed blocked Waiting for another PR/issue to be merged/closed. stale labels Dec 7, 2023
Copy link

github-actions bot commented Feb 6, 2024

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

@github-actions github-actions bot added the stale label Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working easy A quick and easy fix! good first issue Good for newcomers medium priority The default priority for a new issue. stale
Projects
None yet
Development

No branches or pull requests

2 participants