Scripts linter

[rvansa]

A linter would be useful as git commit-hook that will ensure that:

• all scripts have the parameters used declared (as comments) next to the script name
• There's no more than 1 instances of ${{<var>:<default>}}: if the same default is used multiple times this should be extracted to set-state: <var> ${{<var>:<default>}} ahead (or proposed set-default command #160 )
• ... other linting rules?

[johnaohara]

there is already a linter in qDup that validates the structure of the yaml definition and does further analysis (i.e. counting signals etc). you can run it with qdup -T without executing the script, and it is run everytime the script is run.

I think there might be a couple of concerns in this issue that could be split out;

1. additional linting rules. e.g. counting instances of default sets, although there might be situations where you might want different default values depending on context
2. git commit-hook - is the idea here to run the linter on a PR to validate before the changes are committed? if so, I think this is a good idea, we just need a CI workflow to execute qDup -T . This workflow is possible now.
3. all scripts have the parameters used declared (as comments) next to the script name I am not clear on what the ask is here. is this in the context of ensuring that parameters required in qDup are being provided by external qDup wrappers?

atm the linter fails the test if it detects issues (@willr3 please correct me if I am wrong here), we might want to be able to have different levels, e.g. ERROR/WARN/INFO etc. and having different behaviours (i.e. logging vs failure) depending on what levels of issues are discovered.

[willr3]

qDup has -T --test to run static analysis on the scripts to verify they are runnable. it is the same static analysis that qDup performs when starting a run and it checks for signal counts, invalid observing commands (e.g. no sh in a watch) and other requirements.
I think rvansa wants a linter that checks coding styles like the ones that enforce white-space rules in java projects. I can see it being helpful to raise warning for script practices that make them harder to maintain.
using ${key:defaultValue} in multiple places might be easier to maintain with a single set-state: key ${key:defaultValue}.
I'm opposed to style rules for running scripts but I can see a value in a way to check style along with the static analysis rules.
Perhaps a way to add io.hyperfoil.tools.qdup.config.RunRule through the globals?

[rvansa]

Yes, it should be part of the other static analysis.

@johnaohara

1. I don't say that we should enforce same defaults for the variable, but replace full duplicates by single statement
2. We don't do PRs for qDup ATM, so not in CI but on developer's machine instead. Maybe just if you're to commit to master/main branch - that would hint you to use a different branch if you're just quickly testing something WIP.
3. Will started a convention when the parameters passed to scripts are described in the comments, I'd like to enforce that as it seems like a sensible practice improving maintainability.

@willr3 If the static analysis provides just hints/warnings it's most likely to be ignored, there's so much of qDup output... There are two reasons (name others please) why I don't like checkstyle on commit:

• workflow slow down. 5 seconds are acceptable to me, though.
• bothers me during WIP. Therefore I'd apply that only in the main branch - only 'polished' stuff should be committed there. WDYT?