-
Notifications
You must be signed in to change notification settings - Fork 3
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
Deprecate tsv fasta #640
Deprecate tsv fasta #640
Conversation
f412967
to
fb2d185
Compare
a5a86f3
to
e76ceab
Compare
This is a preview of the changelog of the next release. If this branch is not up-to-date with the current main branch, the changelog may not be accurate. Rebase your branch on the main branch to get the most accurate changelog. Note that this might contain changes that are on main, but not yet released. Changelog: 0.4.0 (2024-11-14)⚠ BREAKING CHANGES
FeaturesBug Fixes
|
60a13d3
to
9517a28
Compare
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.
Actually looks good 👍
I would however rephrase the commit message: Instead of "deprecate" I would write something like "remove" (when reading deprecated as a user, I would think it still works, but I should slowly migrate away)
@@ -51,13 +41,7 @@ class PreprocessingConfig { | |||
std::optional<uint32_t> duckdb_memory_limit_in_g; | |||
std::optional<std::filesystem::path> lineage_definitions_file; | |||
std::optional<std::filesystem::path> ndjson_input_filename; |
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.
Can we make this non-optional now?
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.
If this were non-optional, it would need a default value. Anyways, I would wait for the config refactor for this. Maybe it will resolve this already
testBaseData/test_preprocessing_config_with_overridden_defaults.yaml
Outdated
Show resolved
Hide resolved
9517a28
to
ac9fda0
Compare
It is now impossible to specify an input file different from ndjsonInputFilename. An error will be thrown if this variable is not set. resolves #562 BREAKING CHANGE: `metadataInputFile` key in preprocessing config file has been removed. Instead, ndjson files should be used and specified with the `ndjsonInputFilename` option
ac9fda0
to
383b924
Compare
resolves #562
Summary
PR Checklist