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

Improve README and fix compile warnings #54

Merged
merged 2 commits into from Aug 10, 2020
Merged

Conversation

Byron
Copy link
Contributor

@Byron Byron commented Jul 17, 2020

This PR is merely a side-effect of me looking into why my program couldn't display causes using anyhow, so the output looked like this:

Screenshot 2020-07-17 at 14 21 00

A quick investigation showed that anyhow was using the source() method, whereas I and quick-error 1.2.3 still use the now deprecated cause() method.

After cloning the main branch of quick-error, I thought it should be possible to add source() support in a non-breaking fashion. To my surprise, it was already added while removing support for cause(). After migrating by replacing cause(err) with source(err) and description(…) with display(…) all worked as expected and I could witness the error chain I always wanted:

Screenshot 2020-07-17 at 14 27 24

Would it be possible to get a 2.0 release to crates.io?

Alternatively, maybe it's preferred to restore backwards compatibility instead, and I am willing to contribute. Please let me know :), and thanks for your help!

PS

quick-error absolutely has its place in the current error landscape as it is has easy-to-remember syntax and adds only neglectable compile time, while producing minimal error enums suitable for use in library crates. thiserror does something similar, but adds considerable compile time as it's a proc-macro.

Byron added a commit to Byron/gitoxide that referenced this pull request Jul 17, 2020
See tailhook/quick-error#54 for PR
to soon move back to the new release on crates.io
@tailhook
Copy link
Owner

Thanks for PR! I'm going to vacation today, so I'll look in 2-3 weeks.

@Byron
Copy link
Contributor Author

Byron commented Aug 8, 2020

Hi @tailhook , I hope you are doing well, and… sorry to bother you. It's just that gitoxide is in a state where a new release would be nice to share all the cool new things, but that's effectively blocked by quick-error right now.

Sufficient for me would be a 2.0 release of master as is. Alternatively I am offering to re-add support for cause to allow a minor release.

Please let me know what you think :).

@tailhook
Copy link
Owner

Thanks for a reminder.

Alternatively, maybe it's preferred to restore backwards compatibility instead, and I am willing to contribute. Please let me know :), and thanks for your help!

What is the proposed approach?

I'm kinda resistant to 2.0 because we have a trait that would be harder to use if we have a 1.0/2.0 version split. But if that's to heavyweight, we may probably try to do 2.0 anyway.

@tailhook tailhook merged commit a236246 into tailhook:master Aug 10, 2020
@tailhook
Copy link
Owner

PR is fine, though. So merged.

@Byron
Copy link
Contributor Author

Byron commented Aug 11, 2020

What is the proposed approach?

Selfishly I would be opting for a 2.0, however… for the project I absolutely agree it would be preferred to make it a minor version instead by not breaking backwards compatibility, similar to what the standard library does.

I will try to restore the original 'cause' implementation, which I hope to be trivial :D.

@tailhook
Copy link
Owner

Well, just restoring cause probably doesn't solve anything. As it's now deprecated and we eventually have to remove it anyway, similarly to how we've done it to description. I'll take another look at the current state of quick-error this night and do my best to release 2.0.

@Byron
Copy link
Contributor Author

Byron commented Aug 11, 2020

Update: I took a closer look and it seems there are real costs to supporting both versions: more code. As the only thing my limited macro skills allow me to do is to add cause where ever source is used. Probably this was done like this for the same reason - there was no desire to add more code to support deprecated features.

It also appears that I would have to re-add description support, which was dropped here (ca9beae) right before a patch release, and which I consider a breaking change (176beb5).

For the aforementioned reasons I change my stance and instead propose to go for a proper 2.0 which supports the latest Error handling API only, creating only the code necessary to do that.

Alternatively, someone with 'mad macro skillz' could certainly make it so only the necessary code is generated based on understanding the macro input.

Thanks for moving this forward, whatever it maybe, and sorry for not having been able to help more :/ .

@Byron
Copy link
Contributor Author

Byron commented Aug 11, 2020

@tailhook Probably we were composing messages at the same time, and getting that 2.0 release sounds absolutely swell!

Here a quick video of what I can release with quick-error 2.0: gitoxide indexing the linux kernel (aka clone) 1.7x faster than git itself :D.

@tailhook
Copy link
Owner

Just released 2.0. Thank you for help!

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.

None yet

2 participants