feat: parse stream and generate stream

clj-commons / clj-yaml

YAML encoding and decoding for Clojure

Other

122 stars 26 forks source link

feat: parse stream and generate stream #15

Closed davidpham87 closed 3 years ago

davidpham87 commented 3 years ago

Following https://github.com/clj-commons/clj-yaml/issues/3, we followed the inline implementation of https://github.com/metosin/muuntaja/pull/94/files to add the suggested feature. #

borkdude commented 3 years ago

@marcomorain @slipset Any thoughts?

borkdude commented 3 years ago

One could wonder why the input is an InputStream for parse-stream but a writer for generate-stream. I'm not sure what is the right answer, but it feels asymmetrical. Maybe the name of the latter function should simply be write-yaml? And maybe the first function should be called read-yaml while accepting a reader (as returned by io/reader)? You could then even do (read-yaml *in*). Turning *in* into an inputstream is hard, while the reverse is not hard.

borkdude commented 3 years ago

Another point of attention: many read functions take an :eof option to indicate the end of the stream. I think that would be good to add as well, or does that not make sense for reading multiple values from a stream?

davidpham87 commented 3 years ago

Yes, I agree, it feels asymmetrical. I agree with you it would make more sense to accept a reader. I am open to the suggestion of everyone. I honestly do not know the answer, but we could follow transit-clj which also requires are reader.

borkdude commented 3 years ago

It seems the .load method supports loading from a String, InputStream or Reader:

https://github.com/asomov/snakeyaml/blob/master/src/main/java/org/yaml/snakeyaml/Yaml.java#L415-L441

There is also a .loadAll method which reads all objects from any of these types.

So another option could be to have a load and load-all function that works on all of these types (by doing an instance check + a cast to the right type in order to dispatch to the correct Java overloaded method).

Similarly there is .dump and .dumpAll.

slipset commented 3 years ago

@marcomorain ping?

borkdude commented 3 years ago

Reiterating my previous remarks:

[ ] The types for reading and writing differ. One takes in InputStream, the other takes a writer. Why not both streams, or writer+reader? I suggest that the reading function takes a reader instead of an InputStream since it will be hard to read from stdin by passing *in*, for example, as it is now.
[ ] An :eof option is often supported by reading from readers/streams. E.g. (edn/read {:eof ::eof} *in*).

slipset commented 3 years ago

@davidpham87 if you could address @borkdude's comments, we can move this forward.

slipset commented 3 years ago

@davidpham87 ping?