Add lots of missing parameters on ipfs files

[jcaesar]

Please see the commit messages for what exactly was added.

I'm wondering. With this, files_write has 11 parameters. I think that's a bit much, at least I am not able to easily remember it in my short-term memory. But I do not want to do the same thing as #52, at least not before that is merged. One thing I could to to get the paramter count a bit is to make a

pub struct FilesWriteOpts {
    #[serde(skip_serializing_if = "Option::is_none")]
    pub hash: Option<&'a str>,

    #[serde(rename = "cid-version")]
    pub cid_version: i32,

    pub flush: bool,
}

and then #[serde(flatten) that into FilesMkdir, FilesWrite, and FilesChcid.

The only thing I currently have a use-case for is offset on files_write, I added the rest out of a "while we're at it" mood.

Please let me know what you think and whether this has a chance at being merged.

[ferristseng]

Thanks for this! I think I'm more in favor of #52 right now because it handles optionals a bit more gracefully. One advantage of #52 is you don't have to know the default values for fields you don't care about (e.g. I don't know what cid_version is, but I have to pass it in).

I'm not sure what the status of #52 is, but I think I might merge it since this feature has been requested a number of times and it will provide the scaffolding for implementing something similar with other IPFS calls. If you want to branch off #52, and implement the builder for the files_write call you need, I'll be happy to merge everything in and release a new version

[jcaesar]

Rebased on #52 and added builders for requests with more than one bool (so not for all of ipfs files. Should I, for uniformity?).

Thinking about it, I don't really like derive_builder, especially the way I use it here.

• It returns a Result, but it's always a programming error if its Err. So it should panic?
• If you forget to add a required parameter, the compiler won't tell you.
• The struct update syntax is often times elegant enough: request::FilesLs { path: path, .. Default::default() }
• I would like the api to look something like:

  let res = client.files_mkdir_req()
  .parents(true)
  .flush(false)
  .exec_on("/test/nested/dir" /* required parameter, thus not part of the builder pattern */).await?

  but I guess that would require writing macros?

[ferristseng]

Thinking about it, I don't really like derive_builder, especially the way I use it here.

• It returns a Result, but it's always a programming error if its Err. So it should panic?

Agreed, this is an annoying quirk of the library. I'm a fan of trying to use rust-typed-builder instead, which is better about required args. I think I'm going to see how #52 looks with rust-typed-builder instead.

• If you forget to add a required parameter, the compiler won't tell you.

This is something that rust-typed-builder will do as well, but I also share your concern.

• The struct update syntax is often times elegant enough: request::FilesLs { path: path, .. Default::default() }

This is a good point, and perhaps this is sufficient, and we can put the builder stuff behind a feature flag

I would like the api to look something like

The only reason to avoid doing this right now is that it would require significant effort. I agree that this is the most elegant api, but my understanding is it would require the request object to hold a clone of the client instance.

[ferristseng]

Ok, #52 is merged but I used rust-typed-builder instead, and made it an optional feature. Could you update this PR accordingly? Thanks!

[jcaesar]

I used rust-typed-builder instead, and made it an optional feature

Oh, that's a lot more pleasant. (Although I think it could be a default feature. I suspect that the compiler will get rid of the builder code in most cases.)

I migrated the ipfs files builders to rust-typed-builder as well.

[jcaesar]

Thinking about it: Given that the *_with_options variants of the functions are available, this PR does break backwards compatibility somewhat needlessly. If you want me to, I can revert the changes to files_write and friends, and this could be released as 0.7.3.

[ferristseng]

Thinking about it: Given that the *_with_options variants of the functions are available, this PR does break backwards compatibility somewhat needlessly. If you want me to, I can revert the changes to files_write and friends, and this could be released as 0.7.3.

Yes, this would be great! I think everything else looks good too, but I'm going to test it all at some point next week just to confirm. Thank you 💪

[jcaesar]

I reverted the breaking changes. (I hope. At least there's no changes to examples/mfs.rs left.)

This change should not be breaking anymore, but once it's published, changing structs in ipfs_api::request would be a breaking change, since they're all public now. So the same thing couldn't be done for any more api calls.

If you want me to, I can change the structs in ipfs_api::request that are currently not part of the public api to pub(crate) to hopefully have fewer breaking changes in the future. (I'd sure love it if even the structs that have to be public (like request::Add) could be made so that everyone would have to use default like so Add { pin: Some(example), .. Default::default() } and adding fields would not be a breaking change. Sadly, I have no idea how to do that (Adding a private field #[serde(skip)] #[cfg_attr(feature = "builder", builder(setter(skip), default=()))] _phantom: () does not do the trick.) except for forcing the use of the builder, and that would still only be morally not breaking (technically, the builder type changes, but you're not supposed to write it out…).)

[ferristseng]

Sorry for taking so long, this looks great!

If you want me to, I can change the structs in ipfs_api::request that are currently not part of the public api to pub(crate) to hopefully have fewer breaking changes in the future.

I'd sure love it if even the structs that have to be public (like request::Add) could be made so that everyone would have to use default like so Add { pin: Some(example), .. Default::default() } and adding fields would not be a breaking change. Sadly, I have no idea how to do that (Adding a private field #[serde(skip)] #[cfg_attr(feature = "builder", builder(setter(skip), default=()))] _phantom: () does not do the trick.

Hmm, thanks for raising both of these issues -- I don't think I would have noticed them otherwise. I might release this as a RC for now, and see how things shake up. If more of these things get implemented, I'm hoping that we end up in a scenario where we're only introducing breaking changes when the IPFS actually changes.