Some readability refactorings #833

niklasmohrin · 2021-08-21T22:04:47Z

Hi everyone, thank you all for your work on fd :)

I recently saw the reddit post calling for contributions, so I sat down and toyed around with the codebase a bit. I found some things that I think would improve readability. The main change here is that I had rust-analyzer extract some methods from the long run function, because I really couldn't grasp what was happening in the bigger picture. I also changed the ordering of functions in the main.rs file, see the commit message for rationale.

There are two more commits, I also put my rationale into the commit messages there.

Of course, this is a big diff for rather little change. If you don't like any particular change (or all of them), feel free to point that out to me :)

TODO:

if reviewers like structural changes I did to run, I have to closely read all of the code again to check that nothing got lost in the process / during my rebase (tests all pass, but I would rather be sure)

(somewhat related to #382, although this does not include any of the higher level suggestions)

sharkdp · 2021-08-22T11:19:19Z

Thank you very much for your contribution.

This is exactly the type of change I wanted to see from a ticket like #382. I always feel like I need to apologize for the current state of that method 😄.

Tests currently fail on Rust 1.42. It would be great if you could make it work for 1.42. Otherwise, I'm also fine with increasing the min. supported version.

if reviewers like structural changes I did to run, I have to closely read all of the code again to check that nothing got lost in the process / during my rebase (tests all pass, but I would rather be sure)

That would be great

niklasmohrin · 2021-08-22T18:07:27Z

I always feel like I need to apologize for the current state of that method smile.

It always happens so fast :D

Tests currently fail on Rust 1.42.

here is how that went:

bool::then was introduced in 1.50, which is a rather big leap from 1.42. I personally really like bool::then, but it is not a must have
[T; n]: IntoIterator<Item = T> is from 1.53, which is even later. This one is especially maddening because now the tests for merge_exitcodes need to either pass [...].iter().copied() or vec![...]. The latter is the better of the two in my opinion (performance is not a priority in tests)

I now replaced everything with 1.42 equivalents, but we should keep these backports in mind for whenever msrv is increased again.

While going through everything again, I noticed that I totally reverted #819 in the original draft. Maybe a regression test should be introduced? Maybe we can have a test that spawns a subprocess and has a mocked ls in $PATH?

Finally, I moved some things around a bit.

sharkdp · 2021-08-22T18:11:43Z

While going through everything again, I noticed that I totally reverted #819 in the original draft. Maybe a regression test should be introduced? Maybe we can have a test that spawns a subprocess and has a mocked ls in $PATH?

That's an option (we do something similar in bat with mocked pager executables). However, what would there be to test? If we really want to avoid a regression concerning busybox, we would need to run fd within a busybox environment somehow. Maybe that can be done by installing something during the CI run... but to be honest, I'm not sure if it's worth the effort.

src/main.rs

niklasmohrin · 2021-08-22T18:23:14Z

That's an option (we do something similar in bat with mocked pager executables). However, what would there be to test? If we really want to avoid a regression concerning busybox, we would need to run fd within a busybox environment somehow. Maybe that can be done by installing something during the CI run... but to be honest, I'm not sure if it's worth the effort.

I thought that one could just test that the program only gets called with short flags, but I agree that it is an uncommon regression. In that case, it is just something to look out for, especially with these big diffs (I only found it because I looked at the commits done to main since I first started working on this PR). Normal PRs (that is: ones that don't just move huge chunks around) should not have this problem

src/walk.rs

niklasmohrin · 2021-08-31T20:09:41Z

I had read all of the run logic (and found something I had missed before!), but at least one other person should closely check too. I can also check a second time if you want :)

sharkdp · 2021-10-08T20:43:30Z

@niklasmohrin I know it's been a while but it would be great if we could get this ready for merging. I kind of don't want to merge other things before this is done. It would be fantastic if you could do a final round of updates (see my other comment) and solve the merge conflict. Thank you!

For the future I suggest we rather split things up into multiple PRs. That would make the whole review and merge process much easier.

niklasmohrin · 2021-10-09T14:31:47Z

I haven't closely checked that nothing got lost in the rebase from 3ba90dd to feb9698, but it seems that none of the code merged since then touches anything from this PR. Well, and the rebase went though cleanly, sooo... 😅

src/main.rs

…xitCode>` This way, callers don't need to collect into a slice / vec.

Now, the top method is `main`, then comes `run`, then the methods used in `run` follow. Generally, a method is always declared somewhere after its first use. This way, you can read the file from top to bottom with a decreasing level of abstraction (you start with very high-level processes like setting the current dir and logic for which ls command to use only comes furher down).

sharkdp

Thank you! Let's merge this 😄

niklasmohrin force-pushed the mainrefactors branch 2 times, most recently from d0c430a to 10a1e5d Compare August 22, 2021 18:01