Skip to content
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

fix rust parts #23

Merged
merged 62 commits into from
Aug 30, 2023
Merged

fix rust parts #23

merged 62 commits into from
Aug 30, 2023

Conversation

chris-ha458
Copy link
Contributor

either the type should change or the operator should.
It seems the operator should change and document the setting information for future reference

https://rust-lang.github.io/rust-clippy/master/index.html#/absurd_extreme_comparisons

@chris-ha458
Copy link
Contributor Author

I would really love a proper contributing.md document styleguide and precommit.

@chris-ha458
Copy link
Contributor Author

I know that cargo clippy --fix --lib -p dolma would fix a lot of them automatically but I assumed that the original developers had a reason to have written the code in the first place so I am looking at each item one by one to avoid any unintended changes.

fix for more idiomatic code.
also, this variable likely would not need to be investigated further than this.
if you think about it we don't even need to keep the response in memory since we are only looking for the size
simpler string formatting
changing the unwrap was more involved but i think it became better
https://rust-lang.github.io/rust-clippy/master/index.html#/unnecessary_unwrap
The long unwrap chain can also be fixed but it's also a matter of error management so i'm keeping it as is
https://rust-lang.github.io/rust-clippy/master/index.html#/single_match
Not all borrows are needless in this context but if the traits are implemented in a way (either by default or by the code) that automatically copies, when the reference is immediately used and never used again, & might be unnecessary
https://rust-lang.github.io/rust-clippy/master/index.html#/needless_borrow
expect CAN be idiomatic but this does not seem to be the case here.
https://rust-lang.github.io/rust-clippy/master/index.html#/expect_fun_call
we're skipping the okay case so we only need to check the Err(e) case
@soldni
Copy link
Member

soldni commented Aug 23, 2023

@rodneykinney can u take a look? thank you!!

Copy link
Member

@rodneykinney rodneykinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good. Thanks for the clean-up!

note that `bits` can (and should) be immutable now. this is because after initialization, no `AtomicU32`s are added nor removed from `bits`
We are instead relying on interior mutability of the `Vec` through the internal type `AtomicU32`

https://doc.rust-lang.org/reference/interior-mutability.html
you can remove the panic here if you still want to run to propagate error somehow from here
we want to retrieve the unwrapped line here.
If it is Ok(line) it is unwrapped.
if it is Err(e) we log the line_number, doc location and error e. end then we break, specifically the line processing loop
we already have descriptive names
consolidate it into one function
? is idiomatic to replace match that is used only to propagate errors
to_string is infallible(cannot fail and checked during conpile) for string literal
we still have the unwrap, which could be removed as well but this is as concise while leaving the unwrap there
@chris-ha458
Copy link
Contributor Author

I know i've been making more changes since the approval, but unless otherwise stated,
most if not all of the changes are definitely or mostly changes that do not affect behavior and just make the code more idiomatic!

A few that actually change behavior are removing unwrap() as much as possible,
but only when there is a clear and obvious way to address or propagate the error.
As far as I'm concerned they are as suitable for merging as any previous commits, but if there are any that is seen as problematic, let me know.

@soldni soldni merged commit ae04b16 into allenai:main Aug 30, 2023
12 checks passed
@soldni
Copy link
Member

soldni commented Aug 30, 2023

@chris-ha458 looks great! I've merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants