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

BioSequences v3 and BioSymbols v5 #72

Merged
merged 5 commits into from
Jun 8, 2022

Conversation

CiaranOMara
Copy link
Member

@CiaranOMara CiaranOMara commented Mar 22, 2022

This PR:

  • increments dependency compatibilities (reimplements a reinterpret of BioSymbols);
  • includes some doctests in the unit tests; and
  • updates doctests for BioSequences v3 and BioSymbols v5 (adjusts a print width).

@CiaranOMara CiaranOMara marked this pull request as draft March 22, 2022 06:15
@codecov
Copy link

codecov bot commented Mar 22, 2022

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.

@CiaranOMara CiaranOMara changed the title Increment compatibilities Increment version Mar 22, 2022
@CiaranOMara CiaranOMara force-pushed the develop branch 2 times, most recently from 6c4faf0 to d39b119 Compare March 22, 2022 10:07
@CiaranOMara CiaranOMara marked this pull request as ready for review March 27, 2022 14:48
@jakobnissen
Copy link
Member

@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

Copy link
Member

@jakobnissen jakobnissen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to increment major version number.

@CiaranOMara
Copy link
Member Author

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

@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
Copy link
Member Author

CiaranOMara commented Apr 29, 2022

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 #44 (comment) 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
Copy link
Member

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

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

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 Add alignment string position support #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)
  3. Cherry-pick the commits from this PR (BioSequences v3 and BioSymbols v5 #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
Copy link
Member

kescobo commented Jun 6, 2022

@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
Copy link
Member

@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
Copy link
Member

kescobo commented Jun 6, 2022

Oh, I get it.

@CiaranOMara
Copy link
Member Author

CiaranOMara commented Jun 7, 2022

@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 CiaranOMara force-pushed the develop branch 2 times, most recently from 3997519 to 20de45e Compare June 7, 2022 04:22
@CiaranOMara CiaranOMara changed the title Increment version BioSequences v3 and BioSymbols v5 Jun 7, 2022
@CiaranOMara CiaranOMara marked this pull request as ready for review June 7, 2022 04:34
@CiaranOMara CiaranOMara marked this pull request as draft June 7, 2022 05:09
@CiaranOMara CiaranOMara marked this pull request as ready for review June 7, 2022 05:12
@CiaranOMara CiaranOMara force-pushed the develop branch 4 times, most recently from 31da4e1 to 82edd24 Compare June 7, 2022 05:27
@CiaranOMara
Copy link
Member Author

I think this is ready for review again.

Copy link
Member

@kescobo kescobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@@ -16,7 +16,7 @@ jobs:
fail-fast: false
matrix:
version:
- '1.0'
- '1.6'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh man, this is long overdue...

@CiaranOMara CiaranOMara merged commit ed08737 into BioJulia:master Jun 8, 2022
CiaranOMara added a commit that referenced this pull request Jun 8, 2022
- Increments BioSequences compatibility to v3.
- Increments BioSymbols compatibility to v5.
- Updates doctests.
- Drops support for julia less than v1.6.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants