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 clippy lints and other minor improvements #688

Merged
merged 19 commits into from
Aug 8, 2024

Conversation

abysssol
Copy link
Contributor

I was planning on creating a pull request to improve ofborg's behavior when requesting reviews, but noticed that clippy was emitting a lot of warnings and an error. So, I decided to start by fixing them, and along the way ended up adding other assorted improvements as well.

The project appears rather outdated and unidiomatic in general. It seems a partial rewrite may be in order. Most of the dependencies should probably be updated or replaced (traitobject isn't sound since it assumes the layout of fat pointers, but won't be fixed since it's unmaintained). Would there be any problems with replacing hubcaps with octocrab?

Notable commits:
07c2ccf
9925e49
5299de3

A test tries to compare equality of `Vec`s, approximately `x.sort() == y.sort()`,
but `sort()` uses in-place mutation, and returns `unit`, not the input `slice`,
so the lists are never actually compared.
It's possible for a call to `File::open()` to succeed even if reading from
that `File` isn't actually possible due to the `File` being a directory.
In such cases, `Lines` can return `Err` infinitely,
causing `filter_map()` to diverge while searching for a nonexistant `Ok`.

This is a semantic change.
`filter_map()` can filter multiple errors before returning lines again,
while `map_while()` will stop iterating entirely at the first `Err`.

However, errors are unlikely at all since the `file` argument is currently
always a preallocated temporary file for collecting `Command` output.
This merely mitigates the worst case if `lines_from_file()` is ever used
in a way where this error could occur.
replace `Vec<String>` with `Vec<&str>` to reduce allocation
all ranges are already iterators
`append(true)` implies `write(true)`
@abysssol
Copy link
Contributor Author

abysssol commented Aug 5, 2024

@cole-h It appears from merged pull requests that you have been the primary ofborg maintainer for some time. Is that still the case?
If so, I would appreciate a review, or at least a timeline on when I could expect a review.
If not, could you point me toward someone who could give a review? I'm not familiar with how pull requests are handled for ofborg.

Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

The project appears rather outdated and unidiomatic in general

I'd agree with that 😆

Would there be any problems with replacing hubcaps with octocrab?

In principle, no, but when I played with that in the past (which was a while ago to be fair), I think it was missing something that would severely handicap ofborg's functionality (not that I can remember what that was now, though... maybe checkruns are unsupported?).

ofborg/test-srcs/maintainers/lib/attrsets.nix Outdated Show resolved Hide resolved
ofborg/src/tasks/eval/nixpkgs.rs Show resolved Hide resolved
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

Diff LGTM.

I'll attempt to deploy this sometime this week -- if all goes smoothly, I'll come back and merge it 🚀

Thanks!!

@abysssol
Copy link
Contributor Author

abysssol commented Aug 7, 2024

By the way, now that you're done with review, would you like me to squash some of the commits to reduce the quantity of minor commits? I could squash some of the minor clippy fixes, and maybe a couple others. I don't know if it matters, though. Are there any guidelines on commit quantity and size? Any personal preferences?

Do you have any general feedback on how I structured the commits? Anything I could improve?

Thank you for all your help.

@cole-h
Copy link
Member

cole-h commented Aug 8, 2024

would you like me to squash some of the commits to reduce the quantity of minor commits

No need to squash, I'll just rebase-merge as-is.

Are there any guidelines on commit quantity and size? Any personal preferences?

Do you have any general feedback on how I structured the commits? Anything I could improve?

The commits were good, no specific guidance there.


Been deployed for ~1.5h now and seems to be working fine, so I'm gonna merge it now. Thanks again for your effort!

@cole-h cole-h merged commit 5a4e743 into NixOS:released Aug 8, 2024
2 checks passed
@abysssol abysssol deleted the fix-clippy-lints branch August 8, 2024 21:28
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.

2 participants