-
Notifications
You must be signed in to change notification settings - Fork 230
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
Refactor toolkit.nu #791
Conversation
44aa56e
to
07aed54
Compare
@AucaCoyan If you wanna review 📝 Tested combinations of every flag, every subcommand, as well as the stub locally. |
0da763d
to
50494e8
Compare
- 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
50494e8
to
b0ac04e
Compare
There was a problem hiding this 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
closes #789 |
Great catch and fixed! The unrelated YAML changes were just Prettier.
Added to the description! @fdncred I'm planning to reorganize this into a directory module when I integrate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good!
a01a197
to
76a7752
Compare
I'm not a huge fan of using nupm before it's ready because it may result in endless changes here. |
Only for |
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. |
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 👌🏼 |
a9683d5
to
d8454c5
Compare
I'm fine with trying things out and seeing how they go. No worries. I won't block this. |
5fed72a
to
36d0710
Compare
36d0710
to
6e4a89a
Compare
@fdncred Ready when you are |
The |
To clarify there's no testing yet.
Adding nupm to the GitHub action is prerequisite. I'm already attempting it!
I'll tag you in the next PR then ✌🏼 |
Closes #789 🍻
--and-exit
to exit with error codeSTUB_IDE_CHECK=true