Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Staticcheck fixes #1657

Closed
wants to merge 11 commits into from
Closed

Staticcheck fixes #1657

wants to merge 11 commits into from

Conversation

lespea
Copy link
Contributor

@lespea lespea commented Sep 21, 2024

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
Copy link
Contributor Author

lespea commented Sep 21, 2024

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

Edit. Oh nooooooo

@lespea
Copy link
Contributor Author

lespea commented Sep 21, 2024

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.

@johnkerl
Copy link
Owner

johnkerl commented Oct 19, 2024

@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.

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?

@lespea
Copy link
Contributor Author

lespea commented Oct 21, 2024

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.

@johnkerl
Copy link
Owner

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

johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
* Static-check fixes from @lespea #1657, batch 2/n

* Static-check fixes from @lespea #1657, batch 3/n
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
* Static-check fixes from @lespea #1657, batch 2/n

* Static-check fixes from @lespea #1657, batch 3/n

* Static-check fixes from @lespea #1657, batch 4/n
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
* Static-check fixes from @lespea #1657, batch 2/n

* Static-check fixes from @lespea #1657, batch 3/n

* Static-check fixes from @lespea #1657, batch 4/n

* Static-check fixes from @lespea #1657, batch 5/n
johnkerl added a commit that referenced this pull request Oct 27, 2024
* Static-check fixes from @lespea #1657, batch 2/n

* Static-check fixes from @lespea #1657, batch 3/n

* Static-check fixes from @lespea #1657, batch 4/n

* Static-check fixes from @lespea #1657, batch 5/n

* Static-check fixes from @lespea #1657, batch 6/n
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
johnkerl added a commit that referenced this pull request Oct 27, 2024
@johnkerl
Copy link
Owner

@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 johnkerl closed this Oct 27, 2024
@lespea lespea deleted the staticcheckFixes branch October 28, 2024 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants