Skip to content

refactor: amplicon_covs.py #109

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

Merged

Conversation

raaijmag
Copy link
Contributor

refactor: ViroConstrictor.workflow.scripts.amplicon_covs.py - completely re-wrote the script from scratch
refactor: ViroConstrictor.workflow.scripts.amplicon_arg_parser.py - moved argument parsing logic from amplicon_covs to a different file
refactor: ViroConstrictor.workflow.scripts.init.py - added to import the argument parser to amplicon_covs.py

…ely re-wrote the script from scratch

refactor: ViroConstrictor.workflow.scripts.amplicon_arg_parser.py - moved argument parsing logic from amplicon_covs to a different file
refactor: ViroConstrictor.workflow.scripts.__init__.py - added to import the argument parser to amplicon_covs.py
Copy link

Copy link
Contributor

Choose a reason for hiding this comment

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

So this might be a bit of a nitpick but we probably should change some of the function names and/or classnames in this file before we go to include it in the 1.6.0 release.

I think the structure of this file is already a big improvement when it comes to readability and maintainability in the long term, but using something like ReadDirection as a classname, combined with its description, might cause confusion in future maintenance as this class (and actually the entire script) is not actually dealing with the direction of reads, but the direction of primers.

@florianzwagemaker
Copy link
Contributor

I very much approve of this rework, even without the adjustment in the comment above.
However, the splitting of the argument parsing and the actual logic sets a bit of a precedence that we should commit to in a broader sense than just this script.
If we go this way where argument parsing is gathered in separate files then in my opinion we should do this for all the relevant scripts. Potentially combined with a more centralized approach for shared functions across these scripts.

I'd therefore suggest not directly merging this into dev right now, but instead we go to a separate featurebranch where we work this out further for all the relevant scripts and test if this works in a stable manner for both conda and containerized execution. Once we have that worked out then we can include it in the release plan for v1.6.0 so this improvement can finally be included.

@florianzwagemaker florianzwagemaker changed the base branch from dev to refactor_scripts_dir May 26, 2025 17:58
@florianzwagemaker florianzwagemaker merged commit 2736eb6 into refactor_scripts_dir May 26, 2025
2 checks passed
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.

2 participants