miller icon indicating copy to clipboard operation
miller copied to clipboard

Staticcheck fixes

Open lespea opened this issue 1 year ago • 3 comments

So there were a bunch of choices I had to make to get all the checks cleared that I wasn't 100% on so... please make sure to go through everything thoroughly! I tried to group things into similar commits to make it easier to go through; though I somehow messed up git so there's a strange merge commit in there /shrug.

lespea avatar Sep 21 '24 01:09 lespea

Oh dear I missed the regtests... this might take a bit.

Edit. Oh nooooooo

lespea avatar Sep 21 '24 01:09 lespea

Yeah I don't think I have the energy for fixing the reg tests tonight lol. I'll try to finish this tomorrow/this weekend.

lespea avatar Sep 21 '24 01:09 lespea

@lespea thanks for the PR!! This is great stuff!!! :) \

What I would note is this PR is too big -- it tries to do everything all at once. (I do see the 11 separate commits -- thank you!) And there are various regression-test failures which it's a little tricky to attribute to one mod or the other.

Also I'd note that cmd/experiments is sort of a 'shoebox in the attic' I perhaps should have gotten rid of years ago. (Sentimentality, perhaps.)

I've tried splitting this up a bit locally, and here are some notes.

  • Setup:
    • gh pr checkout 1657
    • https://staticcheck.dev/docs/getting-started/
    • go install honnef.co/go/tools/cmd/staticcheck@latest
  • Running the checker:
    • staticcheck ./...

Then:

Directories of files in this PR -- just running dirname on each file on this PR, then running that through sort -u:

.github/workflows
cmd/experiments/colors
cmd/experiments/line_parser
docs/src
docs/src/miller-as-library
man
pkg/bifs
pkg/climain
pkg/dsl
pkg/dsl/cst
pkg/entrypoint
pkg/input
pkg/lib
pkg/mlrval
pkg/output
pkg/parsing/token
pkg/platform
pkg/runtime
pkg/stream
pkg/terminals/regtest
pkg/terminals/repl
pkg/transformers
pkg/types
pkg/version
scripts/early-multi-language-timings
scripts/early-multi-language-timings/mand

Then split-ups like this:

gd -r main -r staticcheckFixes pkg/version > x
patch -p1 < x
make check

Then commit and move on to another directory, etc.

Regenerating the test-expect files: mlr regtest -p followed by git diff

Anyway that's the idea of how to split this up.

I could up separate PRs of my own, the contents of each of which is doing the above, once for each affected directory ... however I'd love to have your name on the PRs that get merged, not mine :)

Here's my suggestion:

  • Do the above (or something like it) for various directories in pkg/. Either multiple PRs, or, split up commits one per subdirectory of pkg/, maybe
  • Do mlr regtest -p on each and check that the resulting diffs are solely due to legit changes made due to the mods to pkg/ -- e.g. the error messages have fewer spaces, or removed ., etc.
  • Put that up

Then separately: I'll take a look separately at your mods to cmd/experiments. Basically these have been just sitting there in source control, never having gotten run, and maybe I should get rid of them ... in any case let's separate your awesome efforts on the mods that affect the used parts of the package (e.g. pkg/) from your awesome efforts on my clutterware.

How does that sound?

johnkerl avatar Oct 19 '24 22:10 johnkerl

Hey sorry this has languished... my life has been pretty crazy as of late and I don't really have time to dedicate to coding non-work things atm. I will revisit this when things settle down but honestly I really don't mind if you just do the splitting yourself and I'm not on the pr's.

lespea avatar Oct 21 '24 22:10 lespea

All good, thanks so much @lespea!!!!!

johnkerl avatar Oct 21 '24 23:10 johnkerl

@lespea multiple PRs split out of this and all amazing! There was a spurious fail on the first commit -- I reverted test files and re-run mlr regtest -p, then mlr regtest twice -- all seems well now. (This seems to have identified the source of the CI failures.)

I don't want to touch the static-check warnings about cmd/experimental right now -- perhaps I'll jettison some of these files later.

Thank you!!!!! :)

johnkerl avatar Oct 27 '24 16:10 johnkerl