Add depth --excl-flags et al flags #1718
Conversation
This matches the behaviour of "view --excl-flags", and is similar to "mpileup --excl-flags". The difference is that mpileup SETs the flags while depth/view OR additional fields onto the flags. (Depth also has -g to AND-off the default exclude flags, which is not needed in view as the default is zero.) Also clarify -g in the usage indicating this is still the code to filter-out rather than filter-in and avoid confusion over "view --incl-flags". This commit does not add a --incl-flags option. Also removed a duplicate -H usage option. Fixes samtools#1702
These match the logic seen in samtools view of needing at least 1 flag and needing all flags to be set.
| fprintf(fp, " Add specified flags to the default filter-out flag list\n"); | ||
| fprintf(fp, " [UNMAP,SECONDARY,QCFAIL,DUP]\n"); | ||
| fprintf(fp, " --incl-flags FLAGS\n"); | ||
| fprintf(fp, " Only inclue records with at least one the FLAGs present [0]\n"); |
There was a problem hiding this comment.
Only incluDe records… (and similarly below).
Also other subcommands accept both ‑‑excl‑flags/‑‑incl‑flags and ‑‑exclude‑flags/‑‑include‑flags.
There was a problem hiding this comment.
Oops. I'll fix the typo.
The include vs incl abbreviation is specific to view. Depth, mpileup, coverage and consensus all have incl-flags but not include-flags. So I think going with the common trend is the right thing here.
There was a problem hiding this comment.
The long option additions to view are the most recent, so I suppose I was trying to introduce a trend of enabling the full spellings. (And hadn't got around to making a PR accepting both interchangeably in the other subcommands.)
But what I really wanted to introduce was samtools/htslib#1361, which would allow unifying all this as a single option instead of adding all these variants to the various subcommands. However the maintainers have carried on adding variant options instead. If there seemed to be interest in the unified approach, I would be happy to dust it off.
There was a problem hiding this comment.
That's because 1361 is draft. When I asked how it was going you said it's still undergoing refactoring, so it's unfair to complain we haven't adopted those policies yet and "have carried on adding variant options instead".
Besides, I always viewed that PR as an additional nomenclature (not "instead") and all new commands would be supporting both old and new syntax for consistency.
There was a problem hiding this comment.
I said “seemed to be interest in [the alternative approach of a unified option]”, not “adopted”.
Certainly I proposed the unified approach as a more user-friendly and mnemonic alternative to further adding to the plethora of flag-filtering options and therefore not adding new variant options (while preserving the already existing ones, of course).
By approving PRs continuing to add to the plethora of flag-filtering options, the maintainers have implicitly signalled that they are not particularly interested in the proposed alternative user interface. Thanks for clarifying.
There was a problem hiding this comment.
The --incl-flags and --require-flags are entirely new to this subcommand, so this part at least was adding new features. Given they're new options they obviously needed some name, and making them compatible with other existing options seemed logical rather than exacerbating things with yet another mnemonic. Please do not take it as a signal that the unified approach is not desired, but as this arrived first there really was no question as to their naming.
See comments in samtools#1718
See comments in samtools#1718
The --excl-flags option is a synonym for -G, and also added --incl-flags and --require-flags to match
viewlogic.Also fixed a minor -H duplication in the usage statement. Fixes #1702.
(Apologies for the late arrival of the PR. I thought I'd already made it,)