build.go: extracts two functions from huge main()

[lkwg82]

Purpose

extracts checkArguments and running commands into two separate functions.

Testing

run the build

[AudriusButkevicius]

This gets lost

[lkwg82]

It seems so, but this diff is a bit poor. Look at line 178 (https://github.com/lkwg82/syncthing/blob/github-checks4/build.go#L178) there it is.

[calmh]

I don't know... There's something about these refactorings that just strokes me the wrong way. I can't fully put my finger on it, but one thing is that you for example extract into functions the fairly trivial single-switch-clause for printing the arch warning, but not the admittedly somewhat hairy get-the-current target function... I would have expected the opposite. I'm also not super keen on passing strings and empty targets to a runCommand function - it seems sane to me call that function with a user supplied argument, but since it's a switch clause where most of the legs are just one function call each, runCommand("vet", target{}) is just a less type safe way of saying vet("cmd", "lib")?

[lkwg82]

I don't know... There's something about these refactorings that just strokes me the wrong way. I can't fully put my finger on it, but one thing is that you for example extract into functions the fairly trivial single-switch-clause for printing the arch warning, but not the admittedly somewhat hairy get-the-current target function... I would have expected the opposite.

Do you remember that this was part of a bigger refactoring? So this is just what I thought of I can do better to ease reading the flow. I tried to keep it focused (from my perspective). Maybe I can do another PR, else this will get bigger again ;).

I'm also not super keen on passing strings and empty targets to a runCommand function - it seems sane to me call that function with a user supplied argument, but since it's a switch clause where most of the legs are just one function call each, runCommand("vet", target{}) is just a less type safe way of saying vet("cmd", "lib")?

Maybe I can go one step further. I'll have another look into.

[lkwg82]

1. I dont know to replace the switch-case properly. Maybe a map of target and functions (actually the same).
2. Else I dont like to inline the vet command call. As it can be run from the default and a specific run. And I like the single command registry approach. We can lookup at a single place, what will be called.
3. runCommand("vet", target{}) mmhm , it can be hidden with a vararg parameter list. This puts some nasty length check code at the top of the func. I'm not sure if you like this. I dont like it. Maybe you tell how you would do it.
4. I just tracked down the usage of tagsargument for install()and build(). This can be inlined in a way. But this wides the PR. We agreed to tradeof refactorings toward many small PRs ;)

So?

[lkwg82]

How to go further?

[calmh]

@st-review merge it, it's fine

build: Extract runCommand from main

[st-review]

👌 Merged as 343dc48. Thanks, @lkwg82!