-
Notifications
You must be signed in to change notification settings - Fork 59
feat: zero main package dependencies, improve tests #57
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
Conversation
Hello, thanks for the PR! There's a lot going on here that I think ideally should have been multiple PRs or commits so they could be reviewed separately:
These changes individually are all nice, though I think the main utility here being the move of tests into a module, which could be done without introducing Since these three changes are commingled, I'll need some extra time to review, and my schedule is already very busy, so I can't promise anything quick. But I will say that if you submit a PR where you only to move the tests, I'd be able to merge it right away. |
Yeah I agree it scope crept a bit as I kept working on it, I don't really want to keep track of a bunch of PRs, but I could rebase into more atomic commits if that would help? As for merge timeline, no rush, it doesn't matter much to me at all for the time being. |
I recommend Aviator or Graphite for PR stacking. 🙂 If you want to split it into commits that'd be nice, sure. |
I'll check them out, do they work on arbitrary public repos? I've used merge queues like BORS before but not apps |
Not sure about Graphite, but the Aviator CLI can be used anywhere without special permissions to the repo and without needing to sign up for Aviator, as it's all client-side. |
@atombender this has been trimmed down to just moving the tests now |
Replaced by #58 apparently |
Yep, wanted to not use |
Summary
tests/
package, which is its own go module, to isolate testing dependencies from the main package