-
Notifications
You must be signed in to change notification settings - Fork 8
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 enable sequence based metadata creation #451
Feature enable sequence based metadata creation #451
Conversation
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.
Thanks for the PR!
Please reuse the fromRows
function to reduce code duplication.
And please undo the styling changes not connected to the specific functions you change, to keep the PR more clean.
Also I would prefer to keep the param : type
styling in general. With the ,
, I'm not sure yet. But I heavily advocate against including this kind of cleanups of code-parts unrelated to the PR.
src/Spreadsheet/ArcAssay.fs
Outdated
|
||
let fromMetadataCollection (collection: seq<seq<string option>>) : ArcAssay = | ||
try | ||
let fromRows (usePrefixes: bool) (rows: seq<SparseRow>) = |
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.
Pull this (fromRows) function out to be used both in fromMetadataSheet
and your function.
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.
fixed
src/Spreadsheet/ArcAssay.fs
Outdated
let rows = | ||
collection | ||
|> Seq.map SparseRow.fromAllValues | ||
let hasPrefix = |
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.
This hasPrefix part can be moved into the fromRows
function and dropped as a param there.
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.
fixed
Fixed the parameter styling and I will avoid style changes in the futur. |
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.
Thanks, looks good now!
Closses #447