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

tidy up classes loading relationship #217

Open
ChristinaXu2017 opened this issue Dec 4, 2020 · 3 comments
Open

tidy up classes loading relationship #217

ChristinaXu2017 opened this issue Dec 4, 2020 · 3 comments

Comments

@ChristinaXu2017
Copy link
Contributor

ChristinaXu2017 commented Dec 4, 2020

Is your feature request related to a problem? Please describe.

  • org.qcmg.pileup.QSnpRecord is a class to store SNP information used by qsnp and qmule project. However, the related file reader is deprecated and only used by qmule which is not well maintained.
  • org.qcmg.protocol.s3* is URL related package only used by qpicard. qpicard is also widely imported by most adamajava projects. At moment any project imports qpicard, automatically imports qio. It cause qio loaded multi times.
  • In current build file of qmotif, qtesting calls qio, however there is no importing of qio classes in these projects.

Describe the solution you'd like

  • We move class QSnpRecord to qcommon, indicates this class is commonly imported by adamajava projects but not IO related.
  • We move org.qcmg.protocol.s3* to qpicard, hence we can remove qio from qpicard build dependency.
  • remove "api project('qio')" from build file of qmotif, qtesting and qpicard
  • add "api project('qio')" to build file of qannotate, due qannotate is no long get qio.jar through qpicard.

Additional context
In this way the relationship between these classes and admajava projects transparent and straight.

@ChristinaXu2017 ChristinaXu2017 changed the title remove IO inrelated classes from qio to qcommon tidy up classes loading relationship Dec 4, 2020
@holmeso
Copy link
Contributor

holmeso commented Dec 7, 2020

I would be inclined to delete QSnpRecord - its been deprecated for a while now.
I'm not sure moving the s3 package to qcommon is a great idea - it is i/o related and I think qio is the best fit for it.

@ChristinaXu2017
Copy link
Contributor Author

ChristinaXu2017 commented Dec 7, 2020

There is a class called QSnpGATKRecord under org.qcmg.common.model, it is similar to QSnpRecord used by qsnp and also deprecated for a while now. Both are IO-related but there are no related Writer or Readers exists. Almost the majority AdamaJava project process files, we can't put all of them under qio, only there are new types of file writers and readers created and then we put them under qio.

Let's keep them at moment, but delete them on the next release together. It requires quite a number of works on qsnp to remove them.

@ChristinaXu2017
Copy link
Contributor Author

We will keep s3 package inside qio. refer to Conrad below suggestion:
@delocalizer delocalizer 8 hours ago Member
I really don't understand this move. The s3 package is for low-level AWS operations - for example, all it ever throws is IOException. picard is specifically for handling fastq/bam/sam sequence data which is a way more specialized task. By moving this out of qio into qpicard that means any time I want to access ANY s3 file stream, whether it contains a VCF, text or whatever, I have to import a sequence data handling package. That just seems wrong to me.
A lot of my reluctance I think comes from my (very imperfect) understanding of what problem this is attempting to solve. It seems like if the issue is multiple paths to the same dependency, that is something that should be fixed in the build configuration (with excludes) rather than restructuring packages themselves.

@ChristinaXu2017 ChristinaXu2017 mentioned this issue Jan 29, 2021
11 tasks
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 a pull request may close this issue.

2 participants