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

Update to edition 2018 #565

Merged
merged 6 commits into from
Jun 1, 2020
Merged

Conversation

pickfire
Copy link
Contributor

@pickfire pickfire commented Dec 19, 2019

Update mdbook to 0.3.5

Command:
sed -i '/extern crate/ {N;s/\n$//}' src/**.md; sed -i 's/extern crate error_chain;/use error_chain::error_chain;/; s/extern crate lazy_static;/use lazy_static::lazy_static;/; s/extern crate bitflags;/use bitflags::bitflags;/; s/extern crate serde_json;/ use serde_json::json;/; s/extern crate serde_derive;/use serde::{Serialize, Deserialize};/; /macro_use/d; /extern crate/ d; s/```rust/```rust,edition2018/; s/bail!/error_chain::&/; s/\(debug\|info\|warn\|error\)!/log::&/;' src/**.md

Fixes #530
Fixes #569

Update mdbook to 0.3.5

Command:
sed -i '/extern crate/ {N;s/\n$//}' src/**.md; sed -i 's/extern crate error_chain;/use error_chain::error_chain;/; s/extern crate lazy_static;/use lazy_static::lazy_static;/; s/extern crate bitflags;/use bitflags::bitflags;/; s/extern crate serde_json;/ use serde_json::json;/; s/extern crate serde_derive;/use serde::{Serialize, Deserialize};/; /macro_use/d; /extern crate/ d; s/```rust/```rust,edition2018/; s/bail!/error_chain::&/; s/\(debug\|info\|warn\|error\)!/log::&/;' src/**.md

Fix rust-lang-nursery#530
@pickfire
Copy link
Contributor Author

@AndyGauge Interested to review?

@AndyGauge
Copy link
Collaborator

Yeah, I was reading it and I'm impressed. One thing I'm concerned about is if there are any extern crate definitions that the text doesn't include a reference. One thing nice about having those extern crate statements is it makes it easy to set up the recipe's Cargo.toml. I'll build it locally and peruse.

@AndyGauge
Copy link
Collaborator

duh, there's crate badges right above the recipes. That should be instructive enough. I've peeked at it and from 30000 ft it looks good.

@pickfire
Copy link
Contributor Author

pickfire commented Dec 19, 2019

@AndyGauge I think the notable changes are index.hbs, the google analytics were removed I guess. Not sure if we need it? For me, I cannot see it, not really transparent so I guess it won't be useful for anyone except those that have access. I wonder why those data are not open sourced?

@pickfire
Copy link
Contributor Author

@AndyGauge What should I do next for this?

@AndyGauge
Copy link
Collaborator

I'm going to try and get it compiled and uploaded to review it in book format next.

@AndyGauge
Copy link
Collaborator

OK, I've published it here http://www.yetanother.site/rust-cookbook/ and am reviewing

src/algorithms/randomness/rand-dist.md Outdated Show resolved Hide resolved
@AndyGauge
Copy link
Collaborator

OK, so I'm now seeing new errors... it looks like error-chain does not like its result type being ignored.

error[E0107]: wrong number of type arguments: expected 2, found 1
--> /tmp/rust-skeptic.HOf7zmrK9Y0r/test.rs:12:14
|
12 | fn main() -> Result<()> {
| ^^^^^^^^^^ expected 2 type arguments
error: aborting due to 2 previous errors
For more information about this error, try rustc --explain E0107.
test about_sect_a_note_about_error_handling_line_150 ... FAILED
error: cannot find macro error_chain_processing in this scope
--> /tmp/rust-skeptic.qfSON6UfVh6g/test.rs:6:1
|
6 | / error_chain! {
7 | | foreign_links {
8 | | Utf8(std::str::Utf8Error);
9 | | AddrParse(std::net::AddrParseError);
10 | | }
11 | | }
| |_^

@AndyGauge
Copy link
Collaborator

AndyGauge commented Feb 24, 2020

Result is a type that is defined from the error-chain! macro, I'm not sure yet why this is happening. When I tested creating a new program from http://www.yetanother.site/rust-cookbook/compression/tar.html#decompress-a-tarball-while-removing-a-prefix-from-the-paths it compiled without errors.

@AndyGauge
Copy link
Collaborator

In order to make Travis useful, I had to fix #569 to figure out what errors are caused by this change.

@AndyGauge
Copy link
Collaborator

OK, here's the problem. error-chain never was updated for Rust 2018 🤕 . Now I can do that, but it looks like here rust-lang/rust#57013 the way to fix error-chain for now is to use all of the internal macros.

use error_chain::{error_chain, error_chain_processing, impl_error_chain_kind, impl_error_chain_processed, impl_extract_backtrace, ChainedError};

Copy link
Contributor Author

@pickfire pickfire left a comment

Choose a reason for hiding this comment

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

Is this fix supposed to be in the 2018 edition branch? saw the comment too late.

By the way, is error_chain still valid for 2018 edition? Isn't something like anyhow suits better now?

@AndyGauge
Copy link
Collaborator

It is really hard to come up with consensus on error handling. I'm biased, because I maintain error-chain (picked up both projects after Brian left Mozilla). I think I would use thiserror in my own library at this point though. Feel free to address error-handling in #502 or #370
The primary reason we don't switch crates is there is no ecosystem stability right now. The cookbook is all about curated, stable libraries that the library team originally approved. Adding crates to the list should be a hard thing. In fact if you want to showcase anyhow as a recipe I'd probably approve it, and throw a thiserror in there too, but for blanket switching all recipes I think that would give me an aneurysm.

@pickfire
Copy link
Contributor Author

pickfire commented May 31, 2020

@AndyGauge What should I do for this pull request?

@AndyGauge
Copy link
Collaborator

We've discovered a major pain point using error-chain with 2018. Either we need to use the internal macros, fix error-chain, or change the error handling library. I believe I wrote those in order of complexity.

I doubt I will be working on this error-chain change immediately, but I think that would be a good solution. If you would rather rewrite the error handling story of the cookbook let me know if you have that capacity.

@AndyGauge
Copy link
Collaborator

Interestingly I discovered that error-chain 0.12 included the 2018 fix. cargo downloads both error-chain 0.11 and 0.12, and incorrectly was using the 0.11 version.

I feel like this is a skeptic problem.

I just recompiled this branch with rustc 1.45-nightly and had every single test fail.

AndyGauge added 2 commits May 31, 2020 15:44
version 4 depends on error-chain v 0.11 and version 5 depends on v 0.12 
!!!!
@AndyGauge
Copy link
Collaborator

OK, let's party like its 2018!

@AndyGauge AndyGauge merged commit b61c8e5 into rust-lang-nursery:master Jun 1, 2020
@pickfire
Copy link
Contributor Author

pickfire commented Jun 1, 2020

I doubt I will be working on this error-chain change immediately, but I think that would be a good solution. If you would rather rewrite the error handling story of the cookbook let me know if you have that capacity.

Yes, I could rewrite the rust error handling story to use anyhow or thiserror.

Also, I would like change the global data story to use once_cell.

But I may not have time recently but should have some time to work this out in few weeks.

@AndyGauge
Copy link
Collaborator

AndyGauge commented Jun 1, 2020 via email

@pickfire
Copy link
Contributor Author

pickfire commented Jun 1, 2020

How about lazy_static? once_cell looks like a better alternative without using macro. https://docs.rs/once_cell/1.4.0/once_cell/#faq

@AndyGauge
Copy link
Collaborator

AndyGauge commented Jun 1, 2020 via email

@pickfire
Copy link
Contributor Author

pickfire commented Jun 1, 2020

I’m fine with a recipe on it. How about highlighting the regex use case? I know @burtsushi describes that as a big part of using either of these, and creating a regex is a good introductory problem. Like https://docs.rs/once_cell/1.4.0/once_cell/#building-block https://docs.rs/once_cell/1.4.0/once_cell/#building-block without creating a macro?

https://docs.rs/once_cell/1.4.0/once_cell/#building-block is creating a macro itself and regex already uses lazy_static, we would have to replace it with once_cell and can also explain the benefits of using once_cell such that one could do lazy initialization without macro.

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.

type annotations needed Update examples to 2018 edition
2 participants