Skip to content

Clean dependencies, fix cross compilation, fix runtime cargo-manifest bug, change from failure to std::error #25

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

Merged
merged 4 commits into from
Dec 22, 2019

Conversation

TheLostLambda
Copy link
Contributor

This also fixes a few typos in the examples.

Overall, this fixes #23, #24, and bumps the library to use new versions of all of it's dependencies.

It now works outside of cargo directories (binaries produced before this fix would crash at runtime if executed outside of a project directory — essentially rendering the library unusable). My solution was to just use an empty organization string, if someone has a better suggestion, please let me know, but the cargo-manifest solution from before was a lot of code that duplicated the user-provided string and broke at runtime.

This also cross-compiles to macOS and the number of dependencies pulled in by this library was cut in third.

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

Overall this looks nice, but could you please split the second commit into several ones?

@TheLostLambda
Copy link
Contributor Author

Sorry for the messy commit, I got a bit carried away and forgot to split it up. That should be a nicer history now!

Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

The commits look good now, but I do not have any experience with the code itself and thus would like to have a second reviewer.

A quick glance did not reveal anything I wouldn't say no to, but as said, I'm not the person to decide this.

@TheLostLambda
Copy link
Contributor Author

Okay, sounds good. I'm using this fork in the meantime. I'll let you know if I encounter any issues, but it's working well on macOS Catelina, Windows 10, Arch Linux, and Linux Mint so far.

@Dylan-DPC-zz
Copy link

Looks good to me. I don't see any red flags while going through the code so merging this

@Dylan-DPC-zz Dylan-DPC-zz merged commit 5a58388 into rust-cli:master Dec 22, 2019
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.

Remove failure dependancy and favor std::error::Error
3 participants