Implement load_from_bytes

[mkmik]

Closes #155

Also helps in some cases with #142, when the BOM is at the beginning of the file (common),
but not in the corner case where the BOM is at the start of a document which is not the first one.

CC @glyn

[mkmik]

Blocked on #139

[glyn]

@mkmik I'd like to understand why this decoder trap is sufficient in all cases. I was expecting it to be necessary to allow the user to choose.

[mkmik]

good point; probably strict is a better default and probably the user would want to be able to override this.

[mkmik]

(changed to Strict)

@glyn what's the sanest way to design an API that would let a user change the decoder error trapping behaviour?

a. Should we make it a mandatory parameter?
b. An Option<>?
c. Two methods, one with defaults and one with some config structure that includes the encoding trap?
d. a "decoder" struct with a "decode" method and a builder pattern to set options:

Decoder::read(file).encoding_trap(mytrap).decode()

?

[glyn]

I quite like (d) as it gives a clean API when you only want the default (strict) behaviour. I could live with (a) or (c). I don't like (b) as it requires an ugly None parameter for the default behaviour.

[chyh1990]

since this MR introduce a new crate dependency, and users can easily implement this helper method in their code. I'm not sure wether we should provide this in yaml crate

[glyn]

since this MR introduce a new crate dependency, and users can easily implement this helper method in their code. I'm not sure wether we should provide this in yaml crate

I think it would be worth including this to ensure consistency of behaviour between users. Also, providing it here makes it more likely that users will consider handling an initial BOM in the first place (rather than finding/fixing a bug).

[mkmik]

yes; more explicitly: it's likely that if this library doesn't do it, the users of this library won't do it either and thus effectively their application won't be compliant with the YAML specification.

The reason I think that's likely, is that in my experience most people either are not aware of this detail of the spec and/or don't think UTF-16 or UTF-8 with BOMs is important at all (e.g. "I never saw some UTF-16 for years, so clearly nobody is using it").

I think adding a small crate dependency is a small price to pay for ensuring that the ecosystem built around this library follows the spec.

[chyh1990]

pls rebase this MR to fix CI

[mkmik]

@glyn I implemented (d), PTAL

[glyn]

@glyn I implemented (d), PTAL

Looks reasonable. One downside of this approach now becomes apparent. In:

let mut d = Decoder::read(file)
let mut e = d.encoding_trap(mytrap)
let f = d.decode()
...

d.encoding_trap(mytrap) mutates d and so f is computed with mytrap, which is surprising if you don't pay attention to the signatures. This is unusual behaviour for the builder pattern. But it's unavoidable if we go with option (d).

[mkmik]

@glyn

let mut d = Decoder::read(file)
let mut e = d.encoding_trap(mytrap)
let f = d.decode()

I'm not a rust expert but that's not how this builder pattern works, I followed https://doc.rust-lang.org/1.0.0/style/ownership/builders.html#consuming-builders: which doesn't use references.

this means that I cannot call d.decode on a value if I already called d.encoding_trap on it.

error[E0382]: use of moved value: `d`
   --> src/yaml.rs:881:19
    |
879 |         let mut d = YamlDecoder::read(s as &[u8]);
    |             ----- move occurs because `d` has type `yaml::YamlDecoder<&[u8]>`, which does not implement the `Copy` trait
880 |         d.encoding_trap(encoding::DecoderTrap::Ignore);
    |         - value moved here
881 |         let out = d.decode().unwrap();
    |                   ^ value used here after move

using mut self initially was confusing to me too; I initially wrote it as:

  pub fn encoding_trap(self, trap: encoding::types::DecoderTrap) -> YamlDecoder<T> {
    let mut new = self;
    new.trap = trap;
    new
  }

but then I saw the official documentation example with mut self in the fn signature, and assumed it was idiomatic rust.

[mkmik]

CI fails because of a transient error

error: caused by: failed to make network request
error: caused by: error sending request for url (https://static.rust-lang.org/dist/channel-rust-stable.toml.sha256): error trying to connect: dns error: No such host is known. (os error 11001)
error: caused by: error trying to connect: dns error: No such host is known. (os error 11001)
error: caused by: dns error: No such host is known. (os error 11001)
error: caused by: No such host is known. (os error 11001)
Command exited with code 1

amending and force pushing to trigger a new CI run

[glyn]

@mkmik mut self is indeed idiomatic rust. It's good the compiler protects us from the issue to some extent. I'm still a bit concerned that encoding_trap() both mutates self and returns self. Perhaps it would be cleaner to give up on chaining and just have it mutate self but not return anything.

  pub fn encoding_trap(self, trap: encoding::types::DecoderTrap) -> YamlDecoder<T> {
    let mut new = self;
    new.trap = trap;
    new
  }

would have issues because self can't necessarily be copied. In particular, the std::io::Read value can't be copied and can only be used once.

[mkmik]

Sorry, I don't understand. This code works:

  pub fn encoding_trap(self, trap: encoding::types::DecoderTrap) -> YamlDecoder<T> {
    let mut new = self;
    new.trap = trap;
    new
  }

if I understand correctly, it's not copying self, but it's transferring ownership to 'new', which is mutable so I can now modify the trap field and return the modified value. The caller of encoding_trap now can't access the value it called encoding_trap on because the value's ownership has been moved.

(sorry if I'm missing the point; I'm unfamiliar with rust in particular, although I have some familiarity with type systems and compiler internals, so almost kinda sorta grasp how this things works, but mostly I have no idea about what's "idiomatic")

[glyn]

@mkmik I'm just getting back into Rust after a long break, so apologies for my lack of understanding. I'm not sure about:

pub fn encoding_trap(self, trap: encoding::types::DecoderTrap) -> YamlDecoder<T> {
    let mut new = self;
    new.trap = trap;
    new
  }

You're quite right that self is moved to new, but I don't particularly like that encoding_trap renders self subsequently unusable. If I find out more, I'll post here. But anyway, let's put that approach to bed either way.

[glyn]

@mkmir @17cupsofcoffee suggested something like the following might be better for a builder:

Suggested change

	pub fn encoding_trap(mut self, trap: encoding::types::DecoderTrap) -> YamlDecoder<T> {
	pub fn encoding_trap(&mut self, trap: encoding::types::DecoderTrap) -> &mut Self {

[mkmik]

Thanks; PTAL

[glyn]

Looks reasonable, thanks. I think it's now up to the project maintainers to decide whether they like the style.

[Diegovsky]

Is this still being worked on?

[glyn]

@SenseTime-Cloud @dtolnay Is anyone looking to merge PRs these days?

[dtolnay]

Not me — this crate is no longer used by serde_yaml.

[davvid]

FWIW I've merged (and various other PRs) this into my fork: https://github.com/davvid/yaml-rust/