search

J35P312 / SVDB

structural variant database software
MIT License
38 stars 16 forks source link

Refactoring #35

Open DanielAndreasen opened 3 years ago

DanielAndreasen commented 3 years ago

To code could use some refactoring. This is quite important, since it hopefully will encourage others to add new features. Refactoring here includes:

dnil commented 3 years ago

Thanks for looking - and I surmise for using the program? 👍🏻 I can't speak for @J35P312 but I have a hard time seeing things like that being anything but most welcome PRs! 😅

DanielAndreasen commented 3 years ago

Yes, we use it. It's a quite cool program :smiley: I added a feature some months ago (only allow variants during query with a frequency lower than a given threshold), and at the same time, I started doing a lot of refactoring. So now I have a local branch called cleanup. I hope we can have a look at this together at some point. Refactoring without testing is a bit risky, so we need good code review.

dnil commented 3 years ago

Sounds great! Right, agreed; for most other projects that got this far we have switched over if not to test driven so at least mandating that each new PR leaves the code more tested than before. That would be lovely here as well..

DanielAndreasen commented 3 years ago

I agree. Maybe we should open a new issue regarding testing? I would say it is going to be easier to start unit testing after refactoring

J35P312 commented 3 years ago

Hello! Yes, I agree, refactoring is really needed, especially for collaboration! Thanks a lot! Feel free to tell us when the cleanup branch is ready, or if you have questions, it sounds excellent!

I have a couple of tests I use to run, like querying a vcf with itself as a db, using strange chromosome names , etc, I can do so once you are ready.

I agree, the code is too messy for unit testing right now.

Thanks again, it will be exciting to hear more about the refactoring.