-
Notifications
You must be signed in to change notification settings - Fork 2
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 first Rust tools #31
Conversation
Rust XML Parser
Rust Functional Analysis
Rust TaxonsUniprots2Tables
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.
In general, I'd avoid using panick to handle problems, rather returning results, and I'd think about how the data should move.
The latter might be a follow-up thing to optimize this, though, since you said this is supposed to be a one-on-one rewrite from the java code.
A lot of the speed in rust code is caused by thinking about data flow. How to avoid copies/clones, trying to use move semantics as much as possible...
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/models.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/taxon_list.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/taxon_list.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/taxon_list.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/bin/functional-analysis.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/bin/functional-analysis.rs
Outdated
Show resolved
Hide resolved
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 mostly follow @ninewise's remarks: using better error handling with Result
's will improve readability of the code. You want to search for everywhere where you:
- do a
match
on aResult
orOption
, - do an
unwrap
orexpect
, - do an
eprinln
followed by an exit.
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/models.rs
Outdated
Show resolved
Hide resolved
scripts/helper_scripts/new-parsers/src/taxons_uniprots_tables/models.rs
Outdated
Show resolved
Hide resolved
Apply suggestions by Felix and Rien
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.
Quick review, don't have much time. I can probably do a bigger one only in the weekend or somewhere next week, so don't wait for my explicit approve to merge.
scripts/helper_scripts/unipept-database-rs/src/bin/xml-parser.rs
Outdated
Show resolved
Hide resolved
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.
There error handling is now way cleaner 👍
There are a few locations where the program would now continue while something fatally wrong has happened (e.g. not being able to write to a file). The program would then spew out lots of error messages without doing anything meaningful. I think in those cases, that should become a fatal error stopping the program.
Excuse the big PR but these were all blocked because of the Rust library we now use, and they ended up merging into one for simplicity. The tools were reviewed individually by Tibo for this reason. Future PRs will do one thing at a time :)
Re-wrote the following tools in Rust:
Added GitHub Actions pipeline for Rust.
I have not yet updated the Dockerfile or
build_database.sh
script, this PR purely adds the new code. We can discuss later (when Pieter is off holiday, so everyone is together) when we'll replace them.