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

Add Project.toml #31

Merged
merged 2 commits into from
Apr 16, 2023
Merged

Conversation

rasmushenningsson
Copy link
Contributor

@rasmushenningsson rasmushenningsson commented Aug 18, 2020

Add Project.toml

Types of changes

This PR implements the following changes:

  • ✨ New feature (A non-breaking change which adds functionality).
  • 🐛 Bug fix (A non-breaking change, which fixes an issue).
  • 💥 Breaking change (fix or feature that would cause existing functionality to change).

📋 Additional detail

  • Updated from old REQUIRE to new Project.toml specification.
  • Fixed some deprecation warnings.
  • Updated travis and appveyor to run tests on Julia 1.5.

☑️ Checklist

  • 🎨 The changes implemented is consistent with the julia style guide.
  • 📘 I have updated and added relevant docstrings, in a manner consistent with the documentation styleguide.
  • 📘 I have added or updated relevant user and developer manuals/documentation in docs/src/.
  • 🆗 There are unit tests that cover the code changes I have made.
  • 🆗 The unit tests cover my code changes AND they pass.
  • 📝 I have added an entry to the [UNRELEASED] section of the manually curated CHANGELOG.md file for this repository.
  • 🆗 All changes should be compatible with the latest stable version of Julia.
  • 💭 I have commented liberally for any complex pieces of internal code.

Also fixed deprecation warnings.
@codecov
Copy link

codecov bot commented Aug 18, 2020

Codecov Report

Merging #31 into master will increase coverage by 28.48%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #31       +/-   ##
===========================================
+ Coverage   56.33%   84.82%   +28.48%     
===========================================
  Files          14       17        +3     
  Lines        1026     1120       +94     
===========================================
+ Hits          578      950      +372     
+ Misses        448      170      -278     
Impacted Files Coverage Δ
src/GeneticVariation.jl 100.00% <ø> (ø)
src/site_counting.jl 60.86% <33.33%> (+35.86%) ⬆️
src/distances/proportion.jl 0.00% <0.00%> (ø)
src/vcf/vcf.jl 100.00% <0.00%> (ø)
src/bcf/bcf.jl 100.00% <0.00%> (ø)
src/allele_freq.jl 94.73% <0.00%> (+1.40%) ⬆️
src/vcf/header.jl 96.55% <0.00%> (+8.55%) ⬆️
src/vcf/writer.jl 100.00% <0.00%> (+12.50%) ⬆️
src/vcf/record.jl 84.78% <0.00%> (+21.40%) ⬆️
... and 9 more

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 f61b9db...06cf100. Read the comment docs.

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.

Any idea why there's such a massive coverage hit? Might be something about how coverage is calculated, I think I remember reading about that. In any case, there's nothing here that should affect it, so I don't think that should hold it up.

Just one minor change (add testing for julia 1.0) from me, and, if you have the energy, taking a look at compat bounds (though I'm fine with leaving those as-is until / unless someone complains).

I don't actually have merge rights on this repo, but looks good AFAICT

IntervalTrees = "524e6230-43b7-53ae-be76-1e9e4d08d11b"
Twiddle = "7200193e-83a8-5a55-b20d-5d36d44a0795"

[compat]
Copy link
Member

Choose a reason for hiding this comment

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

How sure about these compat bounds are you? I suppose we can just set the bounds to current versions, and then test and relax things if anyone complains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've checked and relaxed the compat bounds.
The biggest issue is incompatibility with BioSequences v2. But that requires larger changes and I think that's better to solve in another release. (And a better approach might be to split out a VCF.jl from this package that will simplify the compatibility situation.)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, fixing that seems outside the scope of this PR.

@@ -1,6 +1,6 @@
environment:
matrix:
- julia_version: 1
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to say it's compatible with 1.0, I think we should test on 1.0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed!

@@ -73,11 +73,9 @@ import IntervalTrees: Interval, IntervalValue
import Twiddle:
enumerate_nibbles,
nibble_mask,
count_zero_nibbles,
count_0000_nibbles,
Copy link
Member

Choose a reason for hiding this comment

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

lol

[compat]
Automa = "0.8"
BGZFStreams = "0.3"
BioCore = "2.0.5"
Copy link
Member

Choose a reason for hiding this comment

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

What's going on here? According to SemVer, this should be the same as 2.0, shouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, BioCore has some changes (bug fixes?) that we need. The tests won't pass unless we require at least v2.0.5.

@kescobo
Copy link
Member

kescobo commented Sep 9, 2020

Any idea why there's such a massive coverage hit?

Ignore this - I see that's just the patch 👍

@kescobo
Copy link
Member

kescobo commented Sep 11, 2020

This LGTM - @benjward ?

@biona001
Copy link

biona001 commented Feb 8, 2021

Not to rush anyone, but will this PR be merged? I'm hoping this will solve a lot of the package compatibility issues I'm having with GeneticVariation.

@kescobo
Copy link
Member

kescobo commented Feb 11, 2021

I would, but don't have permission. @benjward bump?

@jonathanBieler
Copy link

Would be great to have this merged yes, I just installed GeneticVariation and saw all my bio packages downgraded a major version or two :/

@rasmushenningsson
Copy link
Contributor Author

I know it has been discussed before, so I decided to move the VCF/BCF functionality to a separate package - VCF.jl. It's currently in the process of being registered.

@CiaranOMara
Copy link
Member

Trusting the review, I'll merge and release this so that things may progress.

@CiaranOMara CiaranOMara merged commit b4789a1 into BioJulia:master Apr 16, 2023
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.

5 participants