✨ add a nu-check verification CI

[AucaCoyan]

Introduction

Hello! I'm tackling #622 that I wanted to be on main branch for a while.
In my dotfiles I had 2 files to know if some new version of nu breaks scripts in nu_scripts, but I couldn't get it really really sharp.
Nonetheless I tried something and did a GitHub action with it. It's on the works, but at least gives results.

How it works:

I made a toolkit.nu with a very bare bones struct, this is called via setup-nu action, and generates the check-files.nu file. After that, another nu instance run that script to check files one by one. The folder before_v0.60/ is excluded.

Some points:

• to know if this PR works, I have to make a PR from AucaCoyan:test into AucaCoyan:add-basic-ci. Only there the CI runs. Link here
• the script of nu check-files.nu needs to return a failure to CI, so it rejects the PR in case of failure. Done
• Imagine this scenario: the CI checks for all the files when you submit a PR to nu_scripts. This means that you would have a failure if some other file than your submits or modifications fails with latest nu version. Wouldn't be better to check only for the files you have modified? How do we do that? With 2 CIs. Solved

Finally

Feel free to submit PRs in my AucaCoyan/nu_scripts repo, pointing to the AucaCoyan:add-basic-ci branch

If any of @fdncred , @amtoine and @sholderbach can give me their opinions/suggestions, I'd be very interested ❤️

When this PR is merged, this fixes #622

[AucaCoyan]

I could check which errors a file give with

nu --ide-check 100 C:\Users\aucac\repos\nu_scripts\sourced\webscraping\shell_stars.nu

[amtoine]

first, thanks for tackling #622 @AucaCoyan 🙏

The folder before_v0.60/ is excluded.

that's good

• to know if this PR works, I have to make a PR from AucaCoyan:test into AucaCoyan:add-basic-ci. Only there the CI runs. Link here

when i look at the CI, it appears all the files are wrong?

• the script of nu check-files.nu needs to return a failure to CI, so it rejects the PR in case of failure

yup that'd be best.

• Imagine this scenario: the CI checks for all the files when you submit a PR to nu_scripts. This means that you would have a failure if some other file than your submits or modifications fails with latest nu version. Wouldn't be better to check only for the files you have modified? How do we do that?

i think that'd be great.
you should be able to git diff <target> --name-only | lines and get the list of files that have been modified in the PR ;)

[AucaCoyan]

Thank you for being so thorough in the review! ❤️
I added the feature to show the errors per file, now the CI prints:

Run nu ./check-files.nu
❌ /home/runner/work/nu_scripts/nu_scripts/custom-menus/current_session_history_menu.nu has errors:
╭───┬────────────┬──────────┬──────────────────────────────┬───────────────────╮
│ # │    type    │ severity │           message            │       span        │
├───┼────────────┼──────────┼──────────────────────────────┼───────────────────┤
│ 0 │ diagnostic │ Error    │ Unexpected end of code.      │ {record 2 fields} │
│ 1 │ diagnostic │ Error    │ Unexpected end of code.      │ {record 2 fields} │
│ 2 │ diagnostic │ Error    │ Unexpected end of code.      │ {record 2 fields} │
│ 3 │ diagnostic │ Error    │ Unexpected end of code.      │ {record 2 fields} │
│ 4 │ diagnostic │ Error    │ Unclosed delimiter.          │ {record 2 fields} │
│ 5 │ diagnostic │ Error    │ Parse mismatch during        │ {record 2 fields} │
│   │            │          │ operation.                   │                   │
╰───┴────────────┴──────────┴──────────────────────────────┴───────────────────╯

❌ /home/runner/work/nu_scripts/nu_scripts/example-config/config.nu has errors:
╭───┬────────────┬──────────┬───────────────────┬───────────────────╮
│ # │    type    │ severity │      message      │       span        │
├───┼────────────┼──────────┼───────────────────┼───────────────────┤
│ 0 │ diagnostic │ Error    │ Module not found. │ {record 2 fields} │
│ 1 │ diagnostic │ Error    │ Module not found. │ {record 2 fields} │
╰───┴────────────┴──────────┴───────────────────┴───────────────────╯
...

I did some legwork so we fix all this lint errors

[AucaCoyan]

Update:
The CI now fails if it finds a file that has errors 💚

[sholderbach]

Do you want check-files.nu to be commited/cached or just generated and used for each workflow run?

[sholderbach]

Surprising to me that the action doesn't trigger so far in ci mode?

[AucaCoyan]

Good! I'm almost there! To recap what this CI does:

• it has 2 workflows, one named ci.yml and one named daily.yml.

  • The first one is geared towards PRs, where it finds the files that had changes in comparison to main.
  • The other I runs once per day and checks all the repo. This is because if a new nushell version is released, it may happen that things that didn't cause an error before now do.

• for the first case, I managed to checkout the files, load nushell in the CI environment, and make a script that it's generated in each CI enviroment named check-files.nu. So, if you run git diff --name-only origin/main | lines | each { path expand } I currently have these files:

C:\Users\AucaMaillo\repos\nu_scripts\.github\workflows\ci.yml
C:\Users\AucaMaillo\repos\nu_scripts\.github\workflows\daily.yml
C:\Users\AucaMaillo\repos\nu_scripts\.gitignore
C:\Users\AucaMaillo\repos\nu_scripts\aliases\bat\bat-aliases.nu
C:\Users\AucaMaillo\repos\nu_scripts\sample.txt
C:\Users\AucaMaillo\repos\nu_scripts\toolkit.nu

How can I filter out the nu files? sometimes we have a few that doesn't have the extension but a shebang (#!) at the start of the file

[sholderbach]

I think we can test with the *.nu files at first. We can then figure out how to efficiently check the files without an extension. (on the nightly check it would probably be more taxing to read all those file headers for the possible shebang variations)

[AucaCoyan]

Allright! Now it's done
You can see the sample with a failing CI here I broke aliases\bat\bat-aliases.nu on purpose to see if it fails correctly
Fixed on the next commit and the CI respond flawlessly

I'm also surprised about the speed of the check 👍🏼

[fdncred]

If the span represents where the error is, it may be good to update your table with the results of view span.

This is the span I'm talking about

╭───┬────────────┬──────────┬──────────────────────────────┬───────────────────╮
│ # │    type    │ severity │           message            │       span        │
├───┼────────────┼──────────┼──────────────────────────────┼───────────────────┤
│ 0 │ diagnostic │ Error    │ Parse mismatch during        │ {record 2 fields} │
│   │            │          │ operation.                   │                   │
│ 1 │ diagnostic │ Error    │ Missing required positional  │ {record 2 fields} │
│   │            │          │ argument.                    │                   │
╰───┴────────────┴──────────┴──────────────────────────────┴───────────────────╯

[sholderbach]

@fdncred are the LSP spans the same as the engine spans? In the given example viewing the particular spans is probably not all that helpful without the context.

I think it is great @AucaCoyan added the listing of errors so the initial triage becomes easier. Either we remove the span column or potentially turn it into line numbers? But reimplementing a whole error reporting seems like a bit much to ask for the initial implementation 😄

[fdncred]

I think the spans should be from the engine, but maybe I'm mistaken.

[AucaCoyan]

I made the tests and apparently you can't get the span once nu --ide-check exits (because the engine is thrown away). I'll remove the span column as it is useless today and a little bit misleading, because it doesn't actually give validated data.
Here is the link to the discord discussion, I copy the samples here to better readability:

I have the pipeline like this:

nu --ide-check 10 '\file\dir\nu_scripts\aliases\bat\bat-aliases.nu' | to text  | ['[', $in, ']'] | str join | from json

returns

  #      type      severity           message               span
─────────────────────────────────────────────────────────────────────
 0   diagnostic   Error      Unexpected end of code.    start   250
                                                        end     250

nu --ide-check 10 '\file\dir\nu_scripts\aliases\bat\bat-aliases.nu' | to text  | ['[', $in, ']'] | str join | from json | get span

gives

 #   start   end
─────────────────
 0     250   250

and

nu --ide-check 10 '\file\dir\nu_scripts\aliases\bat\bat-aliases.nu' | to text  | ['[', $in, ']'] | str join | from json | get span | describe

table<start: int, end: int>

but if I view span it:

$ nu --ide-check 10 '\file\dir\nu_scripts\aliases\bat\bat-aliases.nu' | to text  | ['[', $in, ']'] | str join | from json | get span | view span $in.start $in.end

Error: nu::shell::cant_convert

  × Can't convert to non-negative int.
   ╭─[entry #1:1:160]
 1 │ nu --ide-check 10 '\file\dir\aliases\bat\bat-aliases.nu' | to text  | ['[', $in, ']'] | str join | from json | get span | view span $in.start $in.end
   ·                                                                                                                                                    ─┬─
   ·                                                                                                                                                      ╰── can't convert list<int> to non-negative int
   ╰────

it only works with positive ints, and I am parsing into ints 🤔

[AucaCoyan]

I removed the span column, now the failure in PR 1 is:

Run nu ./check-files.nu
❌ /home/runner/work/nu_scripts/nu_scripts/aliases/bat/bat-aliases.nu has errors:
╭───┬────────────┬──────────┬────────────────────────────╮
│ # │    type    │ severity │          message           │
├───┼────────────┼──────────┼────────────────────────────┤
│ 0 │ diagnostic │ Error    │ Extra positional argument. │
│ 1 │ diagnostic │ Error    │ Unexpected keyword.        │
╰───┴────────────┴──────────┴────────────────────────────╯

✔ /home/runner/work/nu_scripts/nu_scripts/toolkit.nu is ok
💚 All files checked!

[sholderbach]

Awesome! Let's ship it and refine whatever we need later

[AucaCoyan]

Thank you! I finally did it 🎉

[fdncred]

This is an interesting check, but I'm really not enjoying seeing (getting emailed) tons of errors in the CI. Seeing this just makes me want to revert it https://github.com/nushell/nu_scripts/actions/runs/8435887290/job/23102249533. Not sure who's supposed to investigate and fix this stuff.

I also get different results when running it locally.

I also just noticed that it's checking files in the .git folder, which doesn't seem correct.

[sholderbach]

The main branch check broke specifically with #791 maybe worth looking if the file selection got slightly borked in def "with file" (thats too creative naming for my taste). Or whatever was done to address #789 got us into trouble.

I think the solomonic solution would be to just disable the daily workflow, but keep the PR CI.

For stability testing of nushell itself it will be paramount that we can run something like this on a corpus of working nu code. And then the mitigation of breaking changes will be the responsibility of whoever advocates for breaking changes.

[AucaCoyan]

Yes, you are right Stefan. I delivered a number of fixes, but there is still a lot to do to fix main branch entirely.
@fdncred if it's not too much to ask, is it possible to you to disable email notifications for this repo? (maybe for CI only? is that even configurable?)
The motivation for me to do this was that almost with every release of nushell some script of this repo broke, and I usually created a PR manually to fix it. This is for the scripts that I personally use in my config.nu, other people might use different and expect different outcomes.
The CI will not fix the scripts automatically and there is a backlog of old scripts undoubtedly, those will be there even if we turn off the CI. I'm a bit doubtful of what good to us will disable the daily CI do.
What do you think about unpinning #622 from the issue list and maybe create a new issue with this topic?

[fdncred]

IMO, if we have a CI that always fails, there's no need for it until we have a repo that isn't always failing. I don't think we want to condition people to ignore CI failures, which is what this will end up doing. Also, there's no need for it to process files in the .git folder, that should be excluded in the glob.

I can definitely see the benefit of having the script, for people to run locally, and for new PRs to have the script ran on them. All that sounds good. I don't see the need of running the script on most everything that isn't old every day.

I do think it's an interesting idea to put a bunch (25?, 50?) of representative scripts in the nushell repository and have this check script run with every PR so we could check if changes in nushell break the scripts. We know this sometimes but other times it's more of a surprise. This could be more important as we close in on 1.0 as a way of ensuring a certain level of backward compatibility.

[AucaCoyan]

Mmm now I see where you are coming from... that sounds better than keeping a failing CI job.