tiny-pilot / tinypilot

Use your Raspberry Pi as a browser-based KVM.
https://tinypilotkvm.com
MIT License
2.89k stars 244 forks source link

Consolidate naming and responsibility of dev scripts #1716

Closed jotaen4tinypilot closed 1 month ago

jotaen4tinypilot commented 5 months ago

The build-python and build-javascript dev scripts technically don’t build anything, but they rather execute tests and static analysis procedures. Therefore, the prefix build- might be a bit surprising. To me, “build” would indicate that something is compiled or assembled. I’d probably find test- more intuitive here.

We also have several separate “check”-prefixed bash scripts that do static analysis. So it might not be totally clear where to put things. E.g., Python style checking is performed via build-python, whereas JavaScript style checking is performed via check-style and not via build-javascript. As we are about to add tests for bash scripts, we will be having the same duality with check-bash and build-bash.

Seeing that we have 20 dev scripts by now, I suggest we take a step back and review their structure altogether, and try to find a more consistent naming and responsibility scheme.

mtlynch commented 5 months ago

Yeah, the naming is not super well-thought-out. It's mainly that the scripts match conventions I use in previous projects, and I'm used to the naming at this point, so I don't re-evaluate.

I agree that it's confusing because there's not a clear pattern on when to use build vs check vs lint.

I think we should come up with a small, simple set of rules that we feel comfortable following and adapting our scripts to match.

jotaen4tinypilot commented 2 months ago

I’ve dabbled a bit with the structure of the scripts, and to me, the following changes would seem sensible:

(Note, the branch doesn’t reflect all of these suggestions yet, it’s more like a rough initial exploration.)

@jdeanwallace what would you think about that proposed structure?

jdeanwallace commented 2 months ago

@jotaen4tinypilot - These changes look good to me.

Move the enable-passwordless-sudo and the 2 install- scripts to a new folder dev-scripts/remote/, to make it clear that they are supposed to run on device (in contrast to all other scripts, which are made to run on the dev machine)

(nit) The word "remote" makes me think of a device not in my local network. Alternative suggestion: "device"?