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

Refactor stdlib-candidate for nupm #790

Merged
merged 4 commits into from
Mar 16, 2024

Conversation

texastoland
Copy link
Contributor

@texastoland texastoland commented Mar 13, 2024

Easier to review individual commits because of renames. Happy to provide DiffNow links if helpful. Involved:

  1. Moving scripts to a subdirectory
  2. Copying nupm.nuon from another directory
  3. Making modules work
  4. Extracting tests
  5. Fixing tests (related to insert implements closure $in inconsistently nushell#12193)

To test first set up nupm then:

nu_scripts on  std-nupm-integration$env.NUPM_REGISTRIES.nupm_test = 'https://raw.githubusercontent.com/texastoland/nupm/registry-std-rfc/registry.nuon'

nu_scripts on  std-nupm-integrationnupm install std-rfc
╭──────────┬───────────────────────────────────────────╮
│ name     │ std-rfc                                   │
│ version  │ 0.1.0                                     │
│ url      │ https://github.com/texastoland/nu_scripts │
│ revision │ 65aa7cc                                   │
│ path     │ stdlib-candidate                          │
│ type     │ git                                       │
╰──────────┴───────────────────────────────────────────╯
Cloning into 'nu_scripts-4a047f13a05fe35393f3a8d73377b02c-65aa7cc'...
remote: Enumerating objects: 8015, done.
remote: Counting objects: 100% (822/822), done.
remote: Compressing objects: 100% (333/333), done.
remote: Total 8015 (delta 538), reused 641 (delta 445), pack-reused 7193
Receiving objects: 100% (8015/8015), 49.72 MiB | 23.12 MiB/s, done.
Resolving deltas: 100% (4605/4605), done.
Note: switching to '65aa7cc'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 65aa7cc Fix nupm test --dir stdlib-candidate
2024-03-12T20:42:49.462|INF|installing package std-rfc

nu_scripts on  std-nupm-integration took 4suse std-rfc set-env

nu_scripts on  std-nupm-integrationset-env -h
Gracefully set an environment variable or merge a nested option.
...etc.

nu_scripts on  std-nupm-integrationnupm test --dir stdlib-candidate
Testing package /Users/texas/Developer/nu_scripts/stdlib-candidate
tests record filter-name predicate ... SUCCESS
tests record filter-value predicate ... SUCCESS
tests record list_merge ... SUCCESS
tests str append ... SUCCESS
tests fs file bulk-rename ... SUCCESS
tests str prepend ... SUCCESS
tests record filter-name text ... SUCCESS
Ran 7 tests. 7 succeeded, 0 failed.

@texastoland
Copy link
Contributor Author

texastoland commented Mar 13, 2024

nupm test passes (output above). I don't understand the failed checks. It's effectively calling:

nu --ide-check 10 stdlib-candidate/std-rfc/mod.nu | $'[($in)]' | from nuon
╭───┬────────────┬──────────┬───────────────────┬─────────────────╮
│ # │    type    │ severity │      message      │      span       │
├───┼────────────┼──────────┼───────────────────┼─────────────────┤
│ 0 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 24 │  │
│   │            │          │                   │ │ end   │ 31 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 1 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 24 │  │
│   │            │          │                   │ │ end   │ 31 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 2 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 46 │  │
│   │            │          │                   │ │ end   │ 52 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 3 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 46 │  │
│   │            │          │                   │ │ end   │ 52 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 4 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 75 │  │
│   │            │          │                   │ │ end   │ 80 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 5 │ diagnostic │ Error    │ Module not found. │ ╭───────┬────╮  │
│   │            │          │                   │ │ start │ 75 │  │
│   │            │          │                   │ │ end   │ 80 │  │
│   │            │          │                   │ ╰───────┴────╯  │
│ 6 │ diagnostic │ Error    │ Module not found. │ ╭───────┬─────╮ │
│   │            │          │                   │ │ start │ 94  │ │
│   │            │          │                   │ │ end   │ 104 │ │
│   │            │          │                   │ ╰───────┴─────╯ │
│ 7 │ diagnostic │ Error    │ Module not found. │ ╭───────┬─────╮ │
│   │            │          │                   │ │ start │ 94  │ │
│   │            │          │                   │ │ end   │ 104 │ │
│   │            │          │                   │ ╰───────┴─────╯ │
╰───┴────────────┴──────────┴───────────────────┴─────────────────╯

But this works:

nu -n -c 'use stdlib-candidate/std-rfc/mod.nu set-env; set-env -h'
Gracefully set an environment variable or merge a nested option.
...etc.

Maybe @AucaCoyan can help me identify next steps to debug the testing script? I did it in Zed without the LSP I'll check VS Code now.

@fdncred
Copy link
Collaborator

fdncred commented Mar 13, 2024

@texastoland This is what I said in discord.
The only time I see nu --ide-check fail like this is when it can't find files to source/use. The fix in vscode is to add folders to include dirs. I think those pass with a special delimiter via a flag.
The help on nu calls it -I include-path and the delimiter between paths is char record_sep.

@AucaCoyan
Copy link
Contributor

Mmmm yes, this is not correct. Let me check what can we do

@texastoland
Copy link
Contributor Author

Should merge cleanly and pass CI after #791. @kubouch or @amtoine invited for review. The only thing I added was the std-rfc package name. I'll PR the test registry after this drops.

@kubouch
Copy link
Contributor

kubouch commented Mar 15, 2024

I'd avoid relying on the nupm registries and online package installation because that's guaranteed to change, the current implementation is just a proof of concept. If online package installation is not requirred, then it's OK to merge. It's hard to tell from the diff.

@texastoland
Copy link
Contributor Author

texastoland commented Mar 15, 2024

If online package installation is not required, then it's OK to merge.

Confirmed can:

  • nupm install std-rfc (online registry)
  • nupm install --path /path/to/stdlib-candidate (local copy)
  • $env.NU_LIB_DIRS ++= /path/to/stdlib-candidate (then use std-rfc)
  • just use /path/to/stdlib-candidate/std-rfc

Easier to review individual commits because of renames. Happy to provide DiffNow links if helpful. Involved:

  1. Moving scripts to a subdirectory
  2. Copying nupm.nuon from another directory
  3. Making modules work
  4. Extracting tests
  5. Fixing tests (related to insert implements closure $in inconsistently nushell#12193)

The individual commits are readable (unlike the diff) and summarized above. The theme of my changes was just moving existing code:

Before:             After:
━ stdlib-candidate  ━ stdlib-candidate
  ┃                   ┣ std-rfc (new module name)
  ┃                   ┃ ┠ mod.nu (not a module before)
  ┃                   ┃ ┃
  ┣ record            ┃ ┣ record
  ┃ ┠ mod.nu          ┃ ┃ ┠ mod.nu (extracted tests)
  ┃ ┖ README.md       ┃ ┃ ┖ README.md
  ┠ fs.nu             ┃ ┠ fs.nu (extracted tests)
  ┠ * .nu             ┃ ┖ *.nu
  ┖ README.md         ┠ README.md
                      ┃
                      ┠ nupm.nuon (nupm package manifest)
                      ┗ tests (nupm test dir)
                        ┠ mod.md (re-exports)
                        ┠ record.nu
                        ┠ fs.nu
                        ┖ *.nu

@texastoland
Copy link
Contributor Author

texastoland commented Mar 15, 2024

@fdncred Rebased and new CI (correctly running and) passing :shipit:

@kubouch
Copy link
Contributor

kubouch commented Mar 16, 2024

OK, I see the std-rfc can be installed locally from this repo. I think we can merge it. It would give us a chance to dogfood nupm for a bit.

If nupm changes such that it breaks the CI, the CI could be configured to fetch the last working commit until the breakage is updated.

@kubouch kubouch merged commit cf88c11 into nushell:main Mar 16, 2024
1 check passed
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.

4 participants