Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Clean some deprecated classes/methods usages in htsjdk #707
Conversation
coveralls
commented
Sep 16, 2016
•
coveralls
commented
Sep 17, 2016
magicDGS
changed the title from
Clean some deprecated classes/methos usages in htsjdk to Clean some deprecated classes/methods usages in htsjdk
Sep 21, 2016
|
Could you have a look, @lbergelson? I'm pinging you because you were working on deprecated classes/methods in #698 and #699. |
coveralls
commented
Sep 21, 2016
lbergelson
self-assigned this
Oct 24, 2016
|
Friendly ping @lbergelson! |
coveralls
commented
Nov 20, 2016
lbergelson
requested changes
Nov 22, 2016
@magicDGS Thanks for these.
I think deleting deprecated classes/methods should be separated into a separate pr since it's a bigger api change and I want to get some feedback from others about it. I'm in favor of removing the ancient codecs and long deprecated methods, but I'd like to get feedback from others about that. We've recently bitten ourselves by removing some ancient deprecated code that it turned out a bunch of GATK dependencies were still using...
| @@ -322,11 +334,13 @@ public static void setMateInformationOnSupplementalAlignment( final SAMRecord su | ||
| /** | ||
| * This method will clear any mate cigar already present. | ||
| + * @deprecated use {@link #setProperPairAndMateInfo(SAMRecord, SAMRecord, List)} instead |
lbergelson
Nov 22, 2016
Contributor
You're recommending the version without a boolean but calling the one with it. Could you make it consistent?
| + /** | ||
| + * @param rec1 | ||
| + * @param rec2 | ||
| + * @param exepectedOrientations |
lbergelson
Nov 22, 2016
Contributor
type: exepectedOrientations -> expectedOrientations. No point in having these java doc lines without any doc though. Could you just remove the params that aren't given any description?
| @@ -36,6 +36,8 @@ | ||
| * @deprecated use getContig() instead | ||
| */ | ||
| @Deprecated | ||
| - public String getChr(); | ||
| + default public String getChr() { |
| @@ -193,10 +193,13 @@ public static FeatureCodec getFeatureCodec(File featureFile) { | ||
| // return new VCFCodec(); | ||
| if (featureFile.getName().endsWith(".bed") || featureFile.getName().endsWith(".BED") ) | ||
| return new BEDCodec(); | ||
| + // TODO: can this commented lines be removed? |
coveralls
commented
Nov 24, 2016
coveralls
commented
Nov 24, 2016
This was referenced Nov 24, 2016
magicDGS
added some commits
Sep 16, 2016
|
I agree with you @lbergelson, we should separate removal from this PR (I didn't remove anything here in my previous commits). I open several PRs for removing deprecated code, and I let here only deprecated javadoc/annotations, and removal of deprecated code inside HTSJDK (in addition to your Back to you for more comments! |
coveralls
commented
Nov 24, 2016
lbergelson
merged commit 6469969
into
samtools:master
Nov 30, 2016
|
@magicDGS |
magicDGS commentedSep 16, 2016
•
edited
Description
I explore some of the deprecated classes/methods when cleaning some of my own code, and I found that there are usages of some of this methods in other parts of htsjdk. This is a PR to clean the usages and to ask for removal of some deprecated classes/methods no longer used. This includes:
Fixes #330Already fixed in other PRFeature.getChr()toFeature.getContig()usages in htsjdk.SRAAccession.isSupported()usages in the tests and change for the recomended one.@Deprecatedand/** @deprecated */{@link }for pointing to new/correct usage.Asking for removal of classes and methods for different packages, including gelitext support,Edited: open a PR for removal and discussion.VariantContextWriterFactoryand others.I divided the PR in several commits according to common changes, to make it easier to review. I think that this will help in the maintenance of the htsjdk library. If the removal of some classes is made effective, this will break compatibility with previous versions.
Checklist