kbknapp / grab-xkcd

Downloads an XKCD comic
Apache License 2.0
18 stars 1 forks source link

Minor feedback on blogpost #1

Open CGMossa opened 4 years ago

CGMossa commented 4 years ago

Great post! It looked so enticing that I went through it very closely.

---

First, there is a difference between Args in blogpost and in the reference repo (this one)

num: Option<usize>

in blogpost

num: usize

this difference makes:

    fn run(&self) -> anyhow::Result<()> {
        let url = if let Some(n) = self.args.num {
            format!("{}/{}/info.0.json", BASE_URL, n)
        } else {
            format!("{}/info.0.json", BASE_URL)
        };
[redacted]

invalid.

---

You have this:

grab-xkcd --output yaml

but whilst following, it would be nice to have this instead:

cargo run -- --output yaml

as this will do the same from project root.

---

I am unsure how you have in the blogpost that one can use Result<()> once use anyhow::Result; is stated? I don't think you intend to have use anyhow::Result as Result, as you do use the std::result::Result elsewhere. So, I had to write anyhow::Result everywhere else.

Thanks again!

CGMossa commented 4 years ago

Do I need to explicitly write

use std::convert::TryInto;

in order for

        let resp: ComicResponse = http_client.get(&url).send()?.text()?.try_into()?;

to make sense? Otherwise I get that String has no .try_into().

CGMossa commented 4 years ago

Minor typo:

    todo!("do HTTP GET and save the file!");

should be without the ; as otherwise it will not solve the return-type thing.

CGMossa commented 4 years ago

Also,

    file.write_all(&*body.bytes()?).map_err(|e| e.into())

could (maybe??) be replaced with

        file.write_all(&body.bytes()?).map_err(|e|e.into())

I simply don't get the &*-trick. Is it to force a copy?

This is the last thing, I promise. Thanks again.

jhawkesworth commented 4 years ago

Adding my thanks for the blog post, most helpful and I enjoyed the 'code as you go style'

I hit the

num: Option

.. Issue mentioned above when I coded along for myself. Only other teensy bit of feedback I would as it that I spent I bit of time discovering what I had to use so it would be great to mention all the uses needed as you elaborate the code.

Thanks once again