-
Notifications
You must be signed in to change notification settings - Fork 52
Remove convert for mutable BioSequences types. #344
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
base: master
Are you sure you want to change the base?
Remove convert for mutable BioSequences types. #344
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #344 +/- ##
==========================================
+ Coverage 90.87% 91.67% +0.80%
==========================================
Files 31 29 -2
Lines 2400 2823 +423
==========================================
+ Hits 2181 2588 +407
- Misses 219 235 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi maintainers, The failing tests are expected because this PR removes All other tests pass, and coverage is not affected. Thanks! |
src/longsequences/constructors.jl
Outdated
| #Base.convert(::Type{T}, seq::T) where {T <: LongSequence} = seq | ||
| #Base.convert(::Type{T}, seq::T) where {T <: LongSequence{<:NucleicAcidAlphabet}} = seq | ||
|
|
||
| function Base.convert(::Type{T}, seq::LongSequence{<:NucleicAcidAlphabet}) where | ||
| {T<:LongSequence{<:NucleicAcidAlphabet}} | ||
| return T(seq) | ||
| end | ||
| #function Base.convert(::Type{T}, seq::LongSequence{<:NucleicAcidAlphabet}) where | ||
| # {T<:LongSequence{<:NucleicAcidAlphabet}} | ||
| # return T(seq) | ||
| #end |
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 don't think things need to be commented out, they can just be removed. The diff shows what was there/
test/longsequences/conversion.jl
Outdated
| @testset "Object identity" begin | ||
| x = dna"TAG" | ||
| y = LongDNA{4}[] | ||
| push!(y, x) | ||
| @test y[1] === x | ||
| end |
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.
Not sure we actually need this?
|
Thanks for the review, @kescobo!
For the commented-out Base.convert lines in constructors.jl, I left them
temporarily so we could double-check behavior during testing. I can go
ahead and remove them entirely if you’re confident they’re not needed.
Regarding the "Object identity" test in conversion.jl, I added it to ensure
that pushing a LongDNA object into an array preserves object identity
(===). If this seems unnecessary, we can remove it to keep the tests
minimal.
Do you think it’s safe to remove both, or should we keep the identity test
for now?
…On Fri, Dec 5, 2025, 9:08 PM Kevin Bonham ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/longsequences/constructors.jl
<#344 (comment)>
:
> +#Base.convert(::Type{T}, seq::T) where {T <: LongSequence} = seq
+#Base.convert(::Type{T}, seq::T) where {T <: LongSequence{<:NucleicAcidAlphabet}} = seq
-function Base.convert(::Type{T}, seq::LongSequence{<:NucleicAcidAlphabet}) where
- {T<:LongSequence{<:NucleicAcidAlphabet}}
- return T(seq)
-end
+#function Base.convert(::Type{T}, seq::LongSequence{<:NucleicAcidAlphabet}) where
+ # {T<:LongSequence{<:NucleicAcidAlphabet}}
+ # return T(seq)
+#end
I don't think things need to be commented out, they can just be removed.
The diff shows what was there/
------------------------------
In test/longsequences/conversion.jl
<#344 (comment)>
:
> ***@***.*** "Object identity" begin
+ x = dna"TAG"
+ y = LongDNA{4}[]
+ push!(y, x)
+ @test y[1] === x
+end
Not sure we actually need this?
—
Reply to this email directly, view it on GitHub
<#344 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BQ4RRSYFN7YXPKY7Y73IQ3L4AGUXXAVCNFSM6AAAAACOFHPKXKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTKNBVGM4TONZVGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Hi @kescobo Thanks for the feedback! Thanks again! |
This PR removes convert(::T, x) for mutable BioSequence types to prevent unintended implicit conversions that break object identity (===).
Users should now use explicit constructors, e.g., DNASequence(x).
Changes
Removed convert methods for mutable sequences
Updated tests to stop relying on implicit conversion
All tests pass (Pkg.test())
Checklist
Documentation updated (not required unless maintainers prefer)
Tests updated and passing
Added CHANGELOG entry (maintainers can do this)
Fixes #321