BioJulia / BioAlignments.jl

Sequence alignment tools
MIT License
60 stars 24 forks source link

BioSequences v3 and BioSymbols v5 #72

Closed CiaranOMara closed 2 years ago

CiaranOMara commented 2 years ago

This PR:

codecov[bot] commented 2 years ago

Codecov Report

Merging #72 (6ecd4b8) into master (4045711) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   87.95%   87.95%           
=======================================
  Files          16       16           
  Lines        1121     1121           
=======================================
  Hits          986      986           
  Misses        135      135           
Impacted Files Coverage Δ
src/submat.jl 98.29% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 4045711...6ecd4b8. Read the comment docs.

jakobnissen commented 2 years ago

@CiaranOMara I don't see a reason to bump BioAlignments version to 3.0.0, AFAIK, this is not a breaking change. If someone has BioSequences v2 installed, this change will not be breaking, because the Pkg resolver will simply not install this version. Conversely, bumping the major version can cause a bunch of issues for downstream packages that rely on BioAlignments but not BioSequences.

Otherwise LGTM

CiaranOMara commented 2 years ago

The code in this PR doesn't appear to warrant a major version, but since v2 of this package, the method signature for the AlignmentAnchor changed, which warrants the major version change or some sort of patch (reported in #69).

jakobnissen commented 2 years ago

@CiaranOMara Ah, I see. Maybe it's better to revert the breaking change from 2.0.0, since there seem to not be a release that actually includes the change, now that 2.1.0 has been yanked?

CiaranOMara commented 2 years ago

Having had another look to get back up to speed, the breaking change of the adition of the alnpos field to the AlignmentAnchor struct was merged in #44. My sense is that adding alnpos is a useful optimisation, though I've not yet done much in this area. The inclusion of the field is at least important to @alyst.

I'm in favour of moving forward, mainly because #44 was the bulk of the work since v2, but on reflection, it would be worth slowing down to address other breaking changes suggested in https://github.com/BioJulia/BioAlignments.jl/pull/44#issuecomment-697350429 before incrementing a major version number.

If moving forward with alnpos, I think we'd need a few new outer constructors as well as push! and append! methods for Alignment to help with usability by calculating alnpos as Operation and their lengths are added, but these would be features.

I've removed the major version increment and have edited the initial comment of this thread to reflect the changes made.

jakobnissen commented 2 years ago

I think this illustrates very well why most people think that fields of structs should be implementation details, and the API instead should only expose accessor functions - but I digress. You're right - we can have a minor version change for now, then later review what breaking changes we want, if any,

CiaranOMara commented 2 years ago

You're right - we can have a minor version change for now, then later review what breaking changes we want, if any,

We'd need to branch and set a new master before #44 to take/accept minor changes.

MillironX commented 2 years ago

Any progress on this? I'm excited for a new release of BioAlignments, but everything seems to be stuck here. I think this needs to be treated as a hotfix under OneFlow.

  1. Create a new branch before #44
    git checkout -b hotfix/2.2.0 118c5ad3
  2. Cherry-pick the commits from the CI/TagBot PRs (so we can register the new version)
    • 50

    • 71

  3. Cherry-pick the commits from this PR (#72)
    git remote add ciaranomara git@github.com:CiaranOMara/BioAlignments.jl.git
    git fetch ciaranomara
    git cherry-pick c32e7678
    ...
  4. Do version-bumping/testing stuff
  5. Push the hotfix/2.2.0 branch to GitHub
  6. Trigger JuliaRegistrator on the tip of the new branch
  7. Once the version is registered and TagBot has done its thing, then merge hotfix/2.2.0 into master and delete
    git merge hotfix/2.2.0 master
    git push origin :hotfix/2.2.0

I have done steps 1-4 on my own machine, so I know that works as expected, but everything else needs to be done by someone with write access to the repo.

kescobo commented 2 years ago

@MillironX Can you not do 1-5 on a fork and then PR?

I'm happy to take these steps, but don't want to do so until one of @CiaranOMara or @jakobnissen has weighed in

MillironX commented 2 years ago

@MillironX Can you not do 1-5 on a fork and then PR?

I can prepare a PR, but it won't have a target (and it won't show up in GitHub) until someone creates the hotfix branch on this repo.

kescobo commented 2 years ago

Oh, I get it.

CiaranOMara commented 2 years ago

@MillironX, that is an excellent suggestion; I have drafted the hotfix branch and will invite people to review it in the next few days.

I'll tidy up this PR so that it can be merged. After merging this PR into master, we can neatly cherry-pick it into the hotfix.

CiaranOMara commented 2 years ago

I think this is ready for review again.