Extract types

[djc]

Clean up one suppressed clippy lint, with some extra refactoring along the way.

[rbtcollins]

I find this an anti pattern - a single large struct with much more data on it than relevant to the method being called. I forget the code smell from 'refactoring' to describe it more succinctly.

Using a struct to avoid passing a lot of parameters is ok but when we lose visibility about what is used where it becomes hard to detangle in future.

[djc]

IMO this is not "just" using a struct to avoid passing a lot of parameters -- it's actually about passing a more semantically meaningful/higher-level collection of data rather than a bunch of bits and pieces which aren't individually as meaningful. Although I don't disagree in the general case that we don't pass around too much unused state, it appears me to me like the chosen abstraction is quite coherent/tightly coupled anyway, and note that even this caller (the one that's furthest down the stack so far) requires 7 of the 11 fields on the type.

I suppose we could split DistOptions to only contain those 7 fields and leave the other 4 directly in InstallMethod::Dist, would that appease your concerns? IMO that arguably makes the code harder to understand.

[djc]

I've scaled this back a little -- this last step did seem problematic since it needed a a toolchain: ToolchainDesc that could be different from the ToolchainDesc in DistOptions::desc.

[rbtcollins]

I think we are somewhat on this journey already, but would like to make sure we don't build larger aggregates than we actually want - and just doing the mechanical clippy recommendation in this case tends to lead that way.

[djc]

I think we are somewhat on this journey already, but would like to make sure we don't build larger aggregates than we actually want - and just doing the mechanical clippy recommendation in this case tends to lead that way.

IMO for things like type_complexity and too_many_arguments, allowing them at the top of the crate makes it too easy to miss when they're newly triggered in a code review. I don't disagree that the clippy lints are a little overeager, but if anything I prefer them to be locally suppressed in cases where there's no better way to express the code.

On the other hand, in all of the cases here it seems like there's a pretty well-defined collection of data that's already bundled together higher up the stack that's being passed down together, so it makes sense to just keep it bundled as it's being passed down.

[rbtcollins]

I suppose we could split DistOptions to only contain those 7 fields and leave the other 4 directly in InstallMethod::Dist, would that appease your concerns? IMO that arguably makes the code harder to understand.

I think the question I would ask is whether this is a temporary change or part of a sustained investment in detangling/improving/asyncifying (for instance) the code.

If its a tactical response to the clippy lint, then I'm more concerned about setting up poor state management - further embedding the lack of clarity that has accreted over time.

If its just the first commit in some sustained work you'd like to do, I'm more than happy to shelve any concerns about too much state, in the knowledge that you're going to be working on improving that area.

[djc]

I suppose we could split DistOptions to only contain those 7 fields and leave the other 4 directly in InstallMethod::Dist, would that appease your concerns? IMO that arguably makes the code harder to understand.

I think the question I would ask is whether this is a temporary change or part of a sustained investment in detangling/improving/asyncifying (for instance) the code.

In my book this is a clear improvement in the code, and is a form of detangling, because it keeps state that belongs together according to how it's grouped in InstallMethod::Dist together while downloading. A potential further improvement (in my book) would be to move these functions to become functions on DistOptions.

If its a tactical response to the clippy lint, then I'm more concerned about setting up poor state management - further embedding the lack of clarity that has accreted over time.

If its just the first commit in some sustained work you'd like to do, I'm more than happy to shelve any concerns about too much state, in the knowledge that you're going to be working on improving that area.

It is an improvement in my book to stop globally suppressing clippy lints (even ones that are overly eager in terms of sometimes triggering on code that can't really be expressed more cleanly), and I do think this change improves clarity. While I agree that it can be a bad practice to pass around more state than needed, that doesn't seem like a big deal here.

I don't have any grand plans at this point about doing any work in this particular area. I was curious about why the clippy lint was being suppressed and for all of its triggers it seemed that there was an alternative that I preferred anyway, and I cleaned up a bunch of stuff in the process according to what is sometimes named the boy scout rule -- leave the place in better shape than when you found it.

[rami3l]

Oops, sorry for the late review! This looks like a solid improvement in terms of code quality to me, judging by its current state. Thanks :D