Closed duesee closed 4 years ago
In general, don't worry about changing the public API. Changing the API is mostly fine, especially if they're easy to fix and prove their value in terms of improved usability.
I think the public API for parse_response()
should maybe be more like Response::from_bytes()
. For ParseResult
, ideally we'd wrap over nom's IResult
such that nom is no longer part of the public API. types
is already public today. Moving body
, body_structure
and core
into parsers
sounds good to me. It looks like maybe resp_metadata
should be called from the response
parser, so that it can also be part of Response::from_bytes()
(and similar for any parser that results in a variant of Response
).
Some notes on how the process for this should look like:
Does all that make sense?
Yes it absolutely does! Maybe you should paste this into a contributing.md? https://help.github.com/en/github/building-a-strong-community/setting-guidelines-for-repository-contributors
I also think that we should change parse_response
and resp_metadata
, but wanted to focus on other things first. This was the "... for now" part :-)
We could adopt the terminology from the ABNF definitions and expose three parsers:
parse::response::data
(~which I assume is now resp_metadata
~),parse::response::done
, andparse::response::response
(which will invoke data* done
)Exposing all functions may be required, as the server can send "unsolicited" data at any time.
Edit: Okay, resp_metadata
was something different. Anyway, I think that the rest of the comment makes still sense :-)
Edit 2: Ah okay, I see. parse_response -> Response
abstracts over continue, data and done already. That is even better :+1:
So then we're in agreement now on this front at least? :)
Oh, I had the feeling that we agree on most fronts so far :-) Maybe we won't regarding &str and Atom, but I find this really difficult, so not sure if I agree with myself either. The PR for this is not merged yet, but I don't mind if you close this issue as it was a question only. Alternatively I can mention this in some commit.
Let's close this for now, then.
Hey, as you know I am currently in the process of preparing imap-proto for a larger merge. I have started to separate all parsers from type definitions and did some minor cleanups. I tried not to change the public interface, but as this is pre-1.0 maybe we can just make this change now.
How do you feel about exposing the following API:
for now?
Later I think adding
crate::command
, andcrate::response
makes sense. Those can then contain the code frombuilders
, i.e.use crate::command::CommandBuilder
.Currently, also
body
,body_structure
, andcore
are exposed on the top-level. I would like to move them all toparsers::
.What do you think?