-
Notifications
You must be signed in to change notification settings - Fork 418
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: Move binary installation to it's own crate #415
refactor: Move binary installation to it's own crate #415
Conversation
hey @drager looks like this needs a rebase- i've been holding off reviewing because it says WIP- what's the status here? would love to get this merged soon! |
@ashleygwilliams Status is that I'm having some problems with cargo workspaces for the tests. Running However, if I remove the [workspace] section the tests pass... I'm hoping to resolve this as soon as possible and then add some tests for the functions in the |
@ashleygwilliams: I have now resolved the issue with workspaces and I added some tests for the path functions as well. I suppose you could review the code I have pushed now. However, I think some more tests could be added. Please let me know what you think. 😊 |
@drager heads up: if you rebase, then appveyor CI should be fixed 👍 |
@fitzgen: Thank you! 😊 |
The modules and tests are no longer relevant since master has changed a lot since the code was split into smalled modules.
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.
will work on fixing the merge conflict- this looks good to me! thanks so much!
@ashleygwilliams Merge conflicts should now be solved! 🎉 |
ack- i triaged in the wrong order today and looks like they are back :( i'm sorry, i'll be sure to get this in today |
No worries, give me a few minutes and I will resolve them again. |
Now it should be good to go. |
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 |
This PR intends to fix #384.