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 toolkit.nu #791

Merged
merged 5 commits into from
Mar 15, 2024
Merged

Refactor toolkit.nu #791

merged 5 commits into from
Mar 15, 2024

Conversation

texastoland
Copy link
Contributor

@texastoland texastoland commented Mar 14, 2024

Closes #789 🍻

  • Runs without first generating a script
  • Returns error (file) count
  • Requires --and-exit to exit with error code
  • Enables alternative report with env STUB_IDE_CHECK=true
  • Expands documentation
  • All subcommands share same file querying
  • Prepares for nupm test integration

@texastoland
Copy link
Contributor Author

texastoland commented Mar 14, 2024

@AucaCoyan If you wanna review 📝 Tested combinations of every flag, every subcommand, as well as the stub locally.

@texastoland texastoland force-pushed the toolkit-refactor branch 2 times, most recently from 0da763d to 50494e8 Compare March 14, 2024 03:32
- Runs without first generating a script
- Returns error (file) count
- Requires `--and-exit` to exit with error code
- Enables alternative report with env `STUB_IDE_CHECK=true`
- Expands documentation
- All subcommands share same file querying
- Prepares for nupm test integration
Copy link
Contributor

@AucaCoyan AucaCoyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent additions! I'm happier with this code that whatever I wrote in the first place ❤️
Can I ask for you for another change?
on .github/worklows/daily you need to change LN 29 to
nu --commands "use toolkit.nu *; check pr --full --and-exit"
And also remove the last step

I tested this on my repo and it worked as expected (fixes #789 )
You can see the ❌ in the main branch and also the results in the CI

toolkit.nu Outdated Show resolved Hide resolved
@AucaCoyan
Copy link
Contributor

AucaCoyan commented Mar 14, 2024

closes #789
Well... I tried to link this PR to the issue, so, when it's merged it's closed automatically, but github doesn't want to 🤣

@texastoland
Copy link
Contributor Author

on .github/worklows/daily you need to change LN 29 to nu --commands "use toolkit.nu *; check pr --full --and-exit" And also remove the last step

Great catch and fixed! The unrelated YAML changes were just Prettier.

closes #789

Added to the description!

@fdncred I'm planning to reorganize this into a directory module when I integrate nupm test. I have to add nupm to hustcer/setup-nu/issues/40 first.

Copy link
Contributor

@AucaCoyan AucaCoyan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good!

@fdncred
Copy link
Collaborator

fdncred commented Mar 14, 2024

I'm not a huge fan of using nupm before it's ready because it may result in endless changes here.

@texastoland
Copy link
Contributor Author

texastoland commented Mar 14, 2024

I'm not a huge fan of using nupm before it's ready because it may result in endless changes here.

Only for nupm test since the std package is deprecated?

@fdncred
Copy link
Collaborator

fdncred commented Mar 14, 2024

Another reason why I think std testing was removed too soon. If we paint ourselves into a corner here with nupm, we can always remove this functionality from the ci.

@texastoland
Copy link
Contributor Author

texastoland commented Mar 14, 2024

I don't mind deferring it. Provided details on Discord. I'm simplifying the GitHub action (just broke it) then this will be ready to go 👌🏼

@fdncred
Copy link
Collaborator

fdncred commented Mar 14, 2024

I'm fine with trying things out and seeing how they go. No worries. I won't block this.

@texastoland texastoland force-pushed the toolkit-refactor branch 7 times, most recently from 5fed72a to 36d0710 Compare March 14, 2024 18:20
@texastoland
Copy link
Contributor Author

texastoland commented Mar 14, 2024

I'm simplifying the GitHub action (just broke it) then this will be ready to go 👌🏼

@fdncred Ready when you are :shipit:

@kubouch
Copy link
Contributor

kubouch commented Mar 15, 2024

The nupm test is continually tested in nupm's CI, so it should work OK, but its implementation might change, and then the tests would need updating. But it might be worth having nupm being used somewhere for testing and nu_scripts is not so critical, so we could try it and see how it goes.

@kubouch kubouch merged commit 878bfc6 into nushell:main Mar 15, 2024
1 check passed
@texastoland
Copy link
Contributor Author

texastoland commented Mar 15, 2024

@fdncred: I'm fine with trying things out and seeing how they go. No worries. I won't block this.

To clarify there's no testing yet.

[me]: I have to add nupm to hustcer/setup-nu/issues/40 first.

Adding nupm to the GitHub action is prerequisite. I'm already attempting it!

@kubouch: nu_scripts is not so critical, so we could try it and see how it goes.

I'll tag you in the next PR then ✌🏼

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.

main branch CI is not working as expected
4 participants