Update code styling workflow

[sjspielman]

Closes #708

This (draft!) PR updates the code styling workflow to run on only mature modules.

• Currently in my testing, ruff seems to want to look at all code under analyses even with the provided path. I've tried all kinds of quotes combinations...
• The action is not turned on for testing here yet; will do when we think ^ is fixed.
• For now I commented out ewings - is it mature enough to include yet?
• Let me know what we think about the number of branches/PRs. Right now I'm sending each module's code styling to its own branch/PR, because I'm concerned about conflicts from concurrent matrix jobs. I don't want to change our setting of delete-branch: true in the PR step, because we would want to make sure any stale branch is definitely deleted. But at the same time, this setting might risk the branch "deleting itself" via different matrix job. Given this, I thought one branch per module was the safer bet?

[jashapiro]

I think my line comment should solve the issue with picking a directory not working. In essesence I think you are ending up with a command that looks like ruff format <module-dir> . which is then doing both the module directory and the whole repo. I think the fix is just to use the with: src: arg for the action to replace the '.' rather than add to it.

I don't know how exactly I feel about the matrix jobs with multiple PRs. It probably won't be too bad, and it does allow for more focused reviews. But I wonder if we might want to take an alternative approach where we have a file we maintain with a list of the "mature" modules that we could read in an early step, parse, and pass along as an env variable. Then we could have a single set of jobs and a single PR per run if we wanted that.

In practice, that approach might not exclude using a matrix, but we might have to get a bit fancy passing the list of modules from a file to the matrix definition. Pretty sure it is possible though with multiple jobs where only the second job uses a matrix strategy and then using matrix: include: to pass the list of modules to the second job. Syntax might be a bit tricky, but I think it is possible.

A note for implementation of the single-PR version is that I also think you can pass a list of directories to the src: argument in ruff-action as that value gets split internally.

[sjspielman]

In practice, that approach might not exclude using a matrix, but we might have to get a bit fancy passing the list of modules from a file to the matrix definition. Pretty sure it is possible though with multiple jobs where only the second job uses a matrix strategy and then using matrix: include: to pass the list of modules to the second job. Syntax might be a bit tricky, but I think it is possible.

I think this sort of situation is about what you're describing, no? https://github.com/AlexsLemonade/AdminItems/blob/d3feaff26e9b803195b23ee25e2d5c617f3e5f10/.github/workflows/file-annual-creds-rotations.yml#L55-L81

[sjspielman]

Just a note that src: was good for getting ruff to run in the right directories.

[jashapiro]

In practice, that approach might not exclude using a matrix, but we might have to get a bit fancy passing the list of modules from a file to the matrix definition. Pretty sure it is possible though with multiple jobs where only the second job uses a matrix strategy and then using matrix: include: to pass the list of modules to the second job. Syntax might be a bit tricky, but I think it is possible.

I think this sort of situation is about what you're describing, no? https://github.com/AlexsLemonade/AdminItems/blob/d3feaff26e9b803195b23ee25e2d5c617f3e5f10/.github/workflows/file-annual-creds-rotations.yml#L55-L81

Yes, though I would probably just store the list as JSON to avoid the ugly awk

[sjspielman]

Well this is interesting... https://github.com/AlexsLemonade/OpenScPCA-analysis/actions/runs/10618217531/job/29432900775#step:7:61

[jashapiro]

Well this is interesting... https://github.com/AlexsLemonade/OpenScPCA-analysis/actions/runs/10618217531/job/29432900775#step:7:61

80% sure this is because it is running in a fork.

[sjspielman]

I think given the CI checks running in 8a550aa this is probably ready for review. Hopefully the PR wasn't properly filed here for because of the fork situation, since I still don't have other ideas..!

[jashapiro]

I might actually simplify this a bit by making the json file simply look like this:

[
    "hello-R",
    "hello-python",
    "simulate-sce",
    "doublet-detection"
]

and then save not to matrix but directly to a modules output.

Or maybe even simpler for maintenance, use mature-modules.txt:

hello-R
hello-python
simulate-sce
doublet-detection

then:

echo "modules=$(jq -nRc '[inputs]|map(select(length>0))' mature-modules.txt) >> $GITHUB_OUTPUT

(the map/select removes any blank lines, including trailing newline).

I'm also not sure where we should put the mature modules file; where you have it is probably fine for now, but I am not sure it should be buried in a hidden directory.

[sjspielman]

I'm also not sure where we should put the mature modules file; where you have it is probably fine for now, but I am not sure it should be buried in a hidden directory.

I too am not sure about this, and similarly I'm not sure about calling this "mature modules," either, because modules are at different levels of maturity. This list is specifically "what's ready to be styled," so I now think we want a file name that more specifically reflects that too.

[sjspielman]

This should be ready for another look! Let me know what you think about the mature modules list, which I've renamed for now (also do we want ewings in there yet?).

Note that this passed checks here (no PRs filed, but matrix jobs were run): 0e51c5a

[jashapiro]

LGTM, with just a couple of minor suggestions.

[jashapiro]

I would remove this trigger. Since this action files separate PRs, we are never going to want to run it on submitted PRs without substantial changes.