Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature/fixParseCNV #160
base: develop
Are you sure you want to change the base?
Feature/fixParseCNV #160
Changes from 8 commits
5004878
4e23864
2f9b3b3
e9de2c1
cd73f38
33f9eb5
2a80c39
97a7443
37529c8
5a189d3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ik zie dat we vaan verschillende soorten python strings door elkaar gebruiken.
wat er nu staat is niet fout ofzo, maar zouden we misschien voor de nieuwere f-strings kunnen kiezen?
Dan zou deze zin:
if os.path.exists(f"{folder}/SampleSheet.csv"):
worden (en de rest van de strings worden dan ook net anders, maar dat scheelt weer text en het is meestal leesbaarder)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry maar de logica klopt, alleen ik vind het niet heel leesbaar.
als ik het goed begrijp lezen we dus elke lijn van een samplesheet, en als we niet Sample_ID in de regel vinden gaan we door, maar als we in de else terecht komen gaan we een ander codeblock in waarin we dan in sample_section zitten.
ik denk dat een config parser hier de oplossing is:
https://docs.python.org/3/library/configparser.html
dan kan je een config section lezen uit een samplesheet a la
import configparser
config = configparser.ConfigParser()
samplesheet = config.read(samplesheet_path)
je hebt dan 1 sample block met alle samples, en die kan je makkelijk filteren met een for-loopje ofzo.
is een idee, wellicht werkt t niet, maar t proberen waard :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mocht er tijd voor zijn, hou dit stukje dan eens kritisch tegen het licht.
op dit moment zit er namelijk logica in de functie om het aantal samples in een samplesheet te bepalen, maar het doet ook dingen daar buitenom, b.v. een dict bijhouden met het aantal samples per project. eigenlijk mag dat best zijn eigen functie zijn.
en een mooi voorbeeldje van code die heerlijk zou werken met OO!
dan kan je gewoon een run object maken dat 1 of meerdere projects heeft en de determine_samples uitvoeren op een run object, die het op alle (gefilterde) project lijst uitvoert