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
Add barcode logic to CytoSnake's CLI #46
Add barcode logic to CytoSnake's CLI #46
Changes from 6 commits
74f9959
fbd9ffa
32b9f18
6a76c3c
8be9079
8a6bb6a
00d9da6
e5c42a7
5fd8b92
c3d8f41
63b8de8
450f521
1dbbbe0
5ba8421
a3f6331
c0edc18
585e90d
e8c135e
68bd1b4
34c0fbc
22c9f5e
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.
Seeing how patterns in the configs occur (like
compression_options
), consider making use of global variable references (if possible) within these files. These could be referenced within the individual fields to ensure consistency and reduce maintenance in the instances where change is required.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.
Hmmm. It seems like you can assign variables within the configurations and allow re-using: (learned something new today)
Here's an example below:
here's the code that is used to read the test yaml file:
And here's the output:
So it seems like you can but, however, I could see some issues with this in terms of readability. Maybe I am naïve, but I have not seen a config files that uses global variables within them. This begs the question if using config variables is a common trend? For example, do we expect the majority of users to know what these variables are and how it is used within the config file? (I do not know the right answer to this, maybe you, @MattsonCam and/or @gwaybio? Might know this)
I can see this working perfectly in the private configs that exist within the
.cytosnake
directory for development purpose, but I am quite "iffy" about how users will react to variables in config files.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.
Great findings! One way you could consider addressing understandability here would be with comments (for ex:
# this line creates a global config variable used later as *VARIABLE_NAME
). I understand reasons why you might consider not using this method and defer to you on what's best.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.
Double checking: is this block checking whether the barcode file is required or whether it is missing? If it's checking for whether it's missing, consider using "missing" (or similar) in the variable and object names for clarity.
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.
It is checking if the barcode is missing.
Are you suggesting something like this?:
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.
I have done some changes with the naming, let me know if it works.
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.
Thank you for the clarification! In addition to the message, I'm wondering if the exception name itself also should reflect what is "exceptional" or "erroneous". For example,
BarcodeMissingError
or similar. (this might require a change to the exception itself).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.
Ah good point. I'll apply those changes.