gondolyr request list

[tonymushah]

i need to know what feature you wanted to add in the crate.

I have a bunch of stuff 😅 ...
There are probably more I can list but this is already a lot.

• Compile times are painfully slow. As a library, minimizing the compile times should be a priority. Removing external dependencies in favor of building things manually in the project would probably be a good starting place.
  • anyhow and thiserror can be replaced with an error type that is manually implemented rather than derived. See burntsushi's comment https://libreddit.spike.codes/r/rust/comments/gj8inf/rust_structuring_and_handling_errors_in_2020/fqlmknt/?context=3 for some guidelines.
  • derive-builder can easily be done manually and would be good to remove.
• More structs and modules should be made public. This is currently making it impossible for some to compile their programs simply because they can't define the type in their functions.

  error[E0603]: module `manga_aggregate` is private
    --> src/events/components/manga_reader.rs:48:132
     |
  48 | ...ponentInteraction, manga: mangadex_api::v5::schema::manga_aggregate::MangaAggregate) {
     |                                                        ^^^^^^^^^^^^^^^ private module
     |
  note: the module `manga_aggregate` is defined here
    --> /home/jp/.cargo/registry/src/github.com-1ecc6299db9ec823/mangadex-api-1.3.0/src/v5/schema.rs:14:1
     |
  14 | mod manga_aggregate;
     | ^^^^^^^^^^^^^^^^^^^^
  

• The API could be made more ergonomic. The builder pattern felt clunky whenever I tried to use my own library.

  let manga = client
      .manga()
      .aggregate()
      .manga_id(&manga_id)
      .translated_language(languages)
      .build()
      .unwrap()
      .send()
      .await;

Owner Edit: This syntax doesn't work since v3
So the following example should be:

    let manga = client
        .manga()
        .id(your_manga_id)
        .aggregate()
        .get()
        .translated_language(languages)
        .send()
        .await
        .unwrap();

• I never tested the upload functionality. While it seems to compile, it may not actually be possible to upload content. #112
  Owner Edit : It won't work since the old login system is deprecated
• I'm not sure that breaking up the project into workspaces (mangadex-api-types, mangadex-api-schema) was worth the trouble. It might be good to put them back into a single workspace but I can't say which is better honestly.
• The v5 module is probably unnecessary. I made it because of the potential of future major API versions since at the time I made it, there was a v2 and v5 of the MangaDex API.
• Having WASM compatibility would be nice.
• It would be good to see more String fields converted to an enum type where appropriate. Here is one example:

  mangadex-api/mangadex-api-schema/src/v5/check_token_response.rs

  Line 7 in a4da7a1

  pub roles: Vec<String>, // TODO: Deserialize the strings into `UserRole` enum.

• I'm not maintaining this crate for the foreseeable future so I don't need to be included in the list of authors

---

Notes about some of the custom deserialization I added:

• language_array_or_skip_null exists because there are some manga that have null in the availableTranslatedLanguages field. (Example manga: https://api.mangadex.org/manga/a96676e5-8ae2-425e-b549-7f15dd34a6d8)
• localizedstring_array_or_map used to be necessary for the .data.attributes.description field of https://api.mangadex.org/manga/fe38c4c2-8cea-4f91-9e80-b368172bdd5c but it looks like that has been fixed.
• volume_aggregate_array_or_map fixes an issue with some manga aggregate results where the .volumes.0.chapters field would be an array instead of an object. For example, https://api.mangadex.org/manga/d773c8be-8e82-4ff1-a4e9-46171395319b/aggregate. This one has "none" as a key within .volumes: https://api.mangadex.org/manga/14e157c3-7856-4f86-a2d2-8faa86660744/aggregate

Originally posted by @gondolyr in #1 (comment)

[LuMiSxh]

The API could be made more ergonomic. The builder pattern felt clunky whenever I tried to use my own library.

Could you please describe what feels clunky about it?
I'd like to help improve the pattern but since I've not used this wrapper yet (planning to use it in my next project) I can't really make out what feels clunky

[gondolyr]

The API could be made more ergonomic. The builder pattern felt clunky whenever I tried to use my own library.

Could you please describe what feels clunky about it? I'd like to help improve the pattern but since I've not used this wrapper yet (planning to use it in my next project) I can't really make out what feels clunky

In the following example,

let manga = client
    .manga()
    .aggregate()
    .manga_id(&manga_id)
    .translated_language(languages)
    .build()
    .unwrap()
    .send()
    .await;

The build().unwrap() part felt excessive and would be more intuitive to just call send() after building the parameters like the manga ID; this is because of the use of the derive_builder crate (docs). I believe version 3.0.0 refactors this to make it simpler to make requests.

[LuMiSxh]

The API could be made more ergonomic. The builder pattern felt clunky whenever I tried to use my own library.

Could you please describe what feels clunky about it? I'd like to help improve the pattern but since I've not used this wrapper yet (planning to use it in my next project) I can't really make out what feels clunky

In the following example,

let manga = client

    .manga()

    .aggregate()

    .manga_id(&manga_id)

    .translated_language(languages)

    .build()

    .unwrap()

    .send()

    .await;

The build().unwrap() part felt excessive and would be more intuitive to just call send() after building the parameters like the manga ID; this is because of the use of the derive_builder crate (docs). I believe version 3.0.0 refactors this to make it simpler to make requests.

Instead of validating during build(), you could do that during send() and have the unwrap after that and return a Result<>.

But to my knowledge an unwrap is needed somewhere in there, how would you otherwise let the user know something is wrong?

[gondolyr]

The link to burntsushi's comment does not work anymore so here's an alternative: https://red.artemislena.eu/r/rust/comments/gj8inf/rust_structuring_and_handling_errors_in_2020/fqlmknt/?context=3
The article they are referring to is this one here: https://nick.groenen.me/posts/rust-error-handling/

For convenience, I have copied the content here in case the new link goes down as well.

u/burntsushi May 14 '20

Great article! I agree with a lot of it.

I do think it would be wise to be a bit more circumspect with recommendations for thiserror (and similar error handling helper libraries). In particular, they can have a big impact on compilation times. If you're already using proc macros somewhere, or if your dependency tree is already necessarily large, then thiserror seems like a great choice. But otherwise, writing out the impls by hand is very easy to do. I tried it myself and it only took me a couple minutes:

/// WordCountError enumerates all possible errors returned by this library.
#[derive(Debug)]
enum WordCountError {
    /// Represents an empty source. For example, an empty text file being given
    /// as input to `count_words()`.
    EmptySource,

    /// Represents a failure to read from input.
    ReadError { source: std::io::Error },

    /// Represents all other cases of `std::io::Error`.
    IOError(std::io::Error),
}

impl std::error::Error for WordCountError {
    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
        match *self {
            WordCountError::EmptySource => None,
            WordCountError::ReadError { ref source } => Some(source),
            WordCountError::IOError(_) => None,
        }
    }
}

impl std::fmt::Display for WordCountError {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        match *self {
            WordCountError::EmptySource => {
                write!(f, "Source contains no data")
            }
            WordCountError::ReadError { .. } => {
                write!(f, "Read error")
            }
            WordCountError::IOError(ref err) => {
                err.fmt(f)
            }
        }
    }
}

impl From<std::io::Error> for WordCountError {
    fn from(err: std::io::Error) -> WordCountError {
        WordCountError::IOError(err)
    }
}

In terms of compile times, the thiserror version took 5 seconds and 7.5 seconds in debug and release mode, respectively. Without thiserror took 0.37 and 0.39 seconds in debug and release mode, respectively. That's a fairly sizable improvement.
I think using thiserror makes a lot of sense in many circumstances. For example, if I were starting a new project in Rust at work, then using thiserror as a foundation to make quickly building error types in all my internal libraries would be a really nice win. Because writing boiler plate would otherwise (I think) discourage writing out more structured errors, where as thiserror makes it deliciously easy and painless. But I think if I were writing an open source library for others to use, then I think avoiding passing on this compile time hit to everyone else in exchange for a couple minutes of writing some very easy code is well worth it.