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

[MISC] Move parse_cigar from format_sam_base into cigar. #2502

Merged

Conversation

Irallia
Copy link
Contributor

@Irallia Irallia commented Mar 29, 2021

Resolves first part of seqan/iGenVar#85

We decided to move parse_cigar from seqan3::detail::format_sam_base into seqan3/io/sam_file/detail/cigar.hpp.
This is still detail, but now you normaly just need to include one header. Apon this, it is also easier to get everything to do with cigar out of detail later.

@vercel
Copy link

vercel bot commented Mar 29, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/seqan/seqan3/UyjfeBYSUBVkrN1LoQHFHmaoq1H6
✅ Preview: https://seqan3-git-fork-irallia-misc-iomoveparsecigarintocigarhpp-seqan.vercel.app

@codecov
Copy link

codecov bot commented Mar 29, 2021

Codecov Report

Merging #2502 (fe462a6) into master (c40960b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2502   +/-   ##
=======================================
  Coverage   98.22%   98.22%           
=======================================
  Files         269      269           
  Lines       10510    10517    +7     
=======================================
+ Hits        10323    10330    +7     
  Misses        187      187           
Impacted Files Coverage Δ
...lude/seqan3/io/sam_file/detail/format_sam_base.hpp 97.82% <ø> (-0.20%) ⬇️
include/seqan3/io/sam_file/detail/cigar.hpp 97.75% <100.00%> (+0.78%) ⬆️
include/seqan3/io/sam_file/format_bam.hpp 96.21% <100.00%> (ø)
include/seqan3/io/sam_file/format_sam.hpp 97.08% <100.00%> (ø)
include/seqan3/io/record.hpp 100.00% <0.00%> (ø)
include/seqan3/io/sequence_file/input.hpp 100.00% <0.00%> (ø)
include/seqan3/io/sequence_file/output.hpp 100.00% <0.00%> (ø)
include/seqan3/argument_parser/validators.hpp 91.39% <0.00%> (ø)
include/seqan3/io/sequence_file/format_fastq.hpp 100.00% <0.00%> (ø)
include/seqan3/io/sequence_file/input_options.hpp 100.00% <0.00%> (ø)
... and 3 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 c40960b...fe462a6. Read the comment docs.

@Irallia Irallia requested review from a team and simonsasse and removed request for a team March 30, 2021 08:01
@@ -95,11 +95,6 @@ class format_sam_base
header_type & header,
ref_seqs_type & /*tag*/);

static void update_alignment_lengths(int32_t & ref_length,
Copy link
Member

Choose a reason for hiding this comment

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

This static void does not appear in the include/seqan3/io/sam_file/detail/cigar.hpp. Is it just unnecessary ? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As the function is not in a class anymore, I think the static is not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

A static member of a class would mean that the member (in this case a member function) is independent of the actual object. Maybe think of it as "there is one (global) instance of this member" and the member exists without any instance of the class existing.

It also means that you do not need an instance of the format_sam_base (or any derived) class to invoke this function. You can use format_sam_base::update_alignment_lengths instead of some_instance.update_alignment_lengths.

@Irallia Irallia requested a review from simonsasse April 6, 2021 09:43
Copy link
Member

@simonsasse simonsasse left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. Looks good :)

@simonsasse simonsasse requested review from a team and eseiler and removed request for a team April 7, 2021 08:05
@eseiler eseiler enabled auto-merge (squash) April 7, 2021 09:07
@eseiler eseiler merged commit 5d4aa4b into seqan:master Apr 7, 2021
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.

3 participants