-
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
Regionset improvements #96
Conversation
0.2.1 release
Migrate to `upload-artifact@v4`
0.2.2 release
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.
looks good, needs minor cleanup.
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 agree just run cargo fmt
, and then the only real thing I'd wanna discuss further is using Option<String>
for the rest
param instead of an empty string everywhere.
I think that would be better instead of allocated a bunch of empty strings everywhere, right?
So its:
struct Rust {
pub chr: String,
pub start: u32,
pub end: u32,
pub rest: Option<String>
}
Region rest option
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.
lgtm
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 tried to get to it all. The only critical thing is the double import of:
// bindings/python/src/digests/mod.rs
use gtars::digests::{md5, sha512t24u, DigestResult};
Nothing will build or run otherwise. Do you use rust-analyzer
and clippy
? Both are really good at catching things like this.
- Duplication of region to str - \t issues
Improved regionSet
Changes: