BioJulia / FASTX.jl

Parse and process FASTA and FASTQ formatted files of biological sequences.
https://biojulia.dev
MIT License
61 stars 20 forks source link

Implement hashing and equality for Records #10

Closed harryscholes closed 5 years ago

harryscholes commented 5 years ago

Types of changes

This PR implements the following changes: (Please tick any or all of the following that are applicable)

:clipboard: Additional detail

record = FASTA.Record("id", "desc", "ACGT") record2 = FASTA.Record("id", "desc", "ACGT") record1 == record2 hash(record1) == hash(record2) unique([record1, record1, record2, record2]) == [record1] == [record2]

record = FASTQ.Record("id", "desc", "AAGCT", collect("@BCFF")) record2 = FASTQ.Record("id", "desc", "AAGCT", collect("@BCFF")) record1 == record2 hash(record1) == hash(record2) unique([record1, record1, record2, record2]) == [record1] == [record2]


- If you have changed current behaviour...
  - **Describe the behaviour prior to you changes**
Identical `Record`s did not hash to the same value because they are implemented as `mutable struct`s.
  - **Describe the behaviour after your changes** and justify why you have made the changes,
    Please describe any breakages you anticipate as a result of these changes.
Identical `Record`s hash to the same value.

## :ballot_box_with_check: Checklist

- [x] :art: The changes implemented is consistent with the [julia style guide](https://docs.julialang.org/en/stable/manual/style-guide/).
- [ ] :blue_book: I have updated and added relevant docstrings, in a manner consistent with the [documentation styleguide](https://docs.julialang.org/en/stable/manual/documentation/).
- [ ] :blue_book: I have added or updated relevant user and developer manuals/documentation in `docs/src/`.
- [x] :ok: There are unit tests that cover the code changes I have made.
- [x] :ok: The unit tests cover my code changes AND they pass.
- [ ] :pencil: I have added an entry to the `[UNRELEASED]` section of the manually curated `CHANGELOG.md` file for this repository.
- [x] :ok: All changes should be compatible with the latest stable version of Julia.
- [ ] :thought_balloon: I have commented liberally for any complex pieces of internal code.
kescobo commented 5 years ago

Looks pretty good to me. There's one typo in the tests, and I've requested a couple of additional tests for inequality, but this is a great addition. Thanks!

harryscholes commented 5 years ago

Thanks for your review and great ideas, Kevin. I've added those additional tests now

codecov[bot] commented 5 years ago

Codecov Report

Merging #10 into master will increase coverage by 0.43%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #10      +/-   ##
==========================================
+ Coverage   88.32%   88.76%   +0.43%     
==========================================
  Files          13       13              
  Lines         437      445       +8     
==========================================
+ Hits          386      395       +9     
+ Misses         51       50       -1
Impacted Files Coverage Δ
src/fastq/record.jl 82.95% <100%> (+1.47%) :arrow_up:
src/fasta/record.jl 80.41% <100%> (+1.24%) :arrow_up:

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 d718b47...7e835ae. Read the comment docs.

kescobo commented 5 years ago

Is there a way to hit this line with a test?

kescobo commented 5 years ago

👍 Awesome! I don't seem to have merge privileges on this repo, but @BenJWard I've reviewed this and think it looks good.

TransGirlCodes commented 5 years ago

Thank you for prepping this @harryscholes, and thank's for taking the time to go over it @kescobo!