Refactoring modules into `parser::` and `types::` + minor cleanups #80

duesee · 2020-03-25T11:34:10Z

No description provided.

* rearranged modules * moved body and body_structure to parser/ * avoid wildcards "::*" * inlined str import * moved ParseResult

duesee · 2020-03-25T11:35:15Z

I also added an example, so that someone who is unsure wheather to use this library or not can see what it does.

is_ is useful to distinguish between predicates and parsers. This prefix is also used in nom.

djc

So, a bunch of feedback. Note that in general I like this work, and pretty much all of the feedback about how you're presenting it for review. On that front, I'm intentionally on the strict side here so that in the future reviewing will be quicker and we don't have to go round for round. I hope that's okay. Also if you want to push back on any feedback I give that's fine, let's discuss if you disagree!

djc · 2020-03-25T20:51:25Z

imap-proto/src/lib.rs

-// QUOTED-CHAR = <any TEXT-CHAR except quoted-specials> / "\" quoted-specials
-// quoted-specials = DQUOTE / "\"
-// TEXT-CHAR = <any CHAR except CR and LF>
+/// Returns an escaped string if necessary for use as a "quoted" string per


So this (changing comments to docstring) seems unrelated to the goal of the commit (which seems to be reordering code). I also don't see much point in it, since quoted_string() is not exported.

Two points: 1) it is still semantically a docstring (even though it is private and might never be exported in the future) 2) CLion -- and this is the real reason I changed it -- is now able to collapse it ;-)

djc · 2020-03-25T20:51:27Z

imap-proto/src/lib.rs

@@ -63,6 +59,10 @@ fn quoted_string(s: &str) -> Result<Cow<str>, &'static str> {
    }
 }

+#[cfg(test)]


extern crate statements should be near the top IMO. Moving this here doesn't make sense to me.

djc · 2020-03-25T20:52:56Z

imap-proto/src/body_structure.rs

@@ -14,6 +14,22 @@ struct BodyFields<'a> {
    pub octets: u32,
 }

+struct BodyExt1Part<'a> {


This moves the type definitions further away from their single usage sites. That makes this code harder to modify. To me, the parsers that result in these types should probably be treated akin to an impl block, which I always keep immediately below type definitions.

Okay, now that I've seen the follow-up commit that moves the type definitions, I actually understand the goal here. As such, I would prefer to have these two commits squashed together, because this one doesn't make sense on its own (otherwise, please explain any deferred purpose in the commit message).

djc · 2020-03-25T20:57:23Z

imap-proto/src/parser/core.rs

@@ -1,5 +1,4 @@
 use nom::{self, IResult};
-


I like to keep std imports and imports from other crates separate. (Usually these days I put std imports first, then a blank line, then all imports from other crates, then another blank line, then imports from this crate.)

djc · 2020-03-25T20:59:37Z

imap-proto/src/parser/rfc3501/mod.rs

-use crate::body::*;
-use crate::body_structure::*;
-
+use crate::{


djc · 2020-03-25T21:10:40Z

imap-proto/src/parser/rfc5464.rs

@@ -118,7 +117,7 @@ fn entry_name(i: &[u8]) -> IResult<&[u8], &[u8]> {
 }

 fn slice_to_str(i: &[u8]) -> &str {
-    str::from_utf8(i).unwrap()
+    std::str::from_utf8(i).unwrap()


This also appears to belong in an earlier commit.

djc · 2020-03-25T21:10:59Z

tokio-imap/src/client.rs

@@ -13,7 +13,7 @@ use tokio_util::codec::Decoder;

 use crate::proto::{ImapCodec, ImapTransport, ResponseData};
 use imap_proto::builders::command::Command;
-use imap_proto::{Request, RequestId, State};
+use imap_proto::types::{Request, RequestId, State};


Should be fixed in the commit that introduced it.

djc · 2020-03-25T21:13:52Z

imap-proto/src/parser/rfc3501/mod.rs

@@ -23,6 +23,46 @@ use crate::{
 pub mod body;
 pub mod body_structure;

+pub fn parse_response(msg: &[u8]) -> ParseResult {


I don't want this hoisted up top on its own, we should reorder the entire module at once to flow from high-level to low-level.

djc · 2020-03-25T21:14:35Z

imap-proto/src/parser/core.rs

@@ -49,7 +49,7 @@ pub fn quoted_data(i: &[u8]) -> IResult<&[u8], &[u8]> {
 }

 // quoted-specials = DQUOTE / "\"
-pub fn quoted_specials(c: u8) -> bool {
+pub fn is_quoted_specials(c: u8) -> bool {


Good idea. 👍

djc · 2020-03-25T21:17:33Z

imap-proto/src/parser/core.rs

-    data: take!(len) >> // FIXME: 0x00 is not allowed
-    (data)
-));
+/// literal = "{" number "}" CRLF *CHAR8


I'd like to split this into two commits: one commit that converts literal to function-style, and one commit that makes it more strict. Otherwise, it makes reviewing stuff much harder.

duesee · 2020-03-25T22:22:24Z

Thank you for the feedback! Makes all sense :-) There is only one problem... I have no idea how to split commits in a PR. Should I close this one first? I will try tomorrow evening.

djc · 2020-03-26T07:43:45Z

I usually use git rebase -i to do an interactive rebase. If you set that to edit the offending commit, you can then git reset HEAD^ to reset the branch pointer to the previous commit while keeping your changes, so you can make new commits with git commit -p or similar.

duesee · 2020-03-27T14:59:16Z

Okay, I think I learned how to squash, rearrange and split commits :-) See #81. I added 3 more commits here which you can review. Otherwise the new branch is identical to this one. Just so you don't have to review anything twice.

duesee added 12 commits March 22, 2020 16:53

added example for response parsing

72d774d

prepare to move all parsers in parser/mod.rs

fc23a77

try to understand what is exported

6bd3dae

moved types to top of file

ef22962

renamed src/types.rs to src/types/mod.rs

ebcdb0f

moved some structs to types/mod.rs

ae42e1b

moved core.rs to parser/

08423c1

reformatted imports

3c0045b

TODO: use derive(Copy) instead?

fee8ae6

Changed public API (not quite finished)

24a400c

* rearranged modules * moved body and body_structure to parser/ * avoid wildcards "::*" * inlined str import * moved ParseResult

comment in lib.rs

c4ebefd

using core::* is probably most productive

095a992

duesee added 3 commits March 25, 2020 12:40

fixed import in tokio-imap

cb80882

moved some parsers and added "sections"

7eecd1c

Added "is_" prefix to functions in core.rs

5e934d2

is_ is useful to distinguish between predicates and parsers. This prefix is also used in nom.

duesee changed the title ~~Refactoring modules into parser:: and types::~~ Refactoring modules into parser:: and types:: + minor cleanups Mar 25, 2020

fixed #77 and replaced literal macro with nom 5 function

fc7b4c6

djc reviewed Mar 25, 2020

View reviewed changes

duesee added 3 commits March 27, 2020 14:12

just derive(Copy), TODO solved

f43beb4

added is_ prefix to tag_char

0e8a6f9

reverted move of assert_matches and parsers in rfc3501/mod.rs

30de096

duesee closed this Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring modules into `parser::` and `types::` + minor cleanups #80

Refactoring modules into `parser::` and `types::` + minor cleanups #80

duesee commented Mar 25, 2020

duesee commented Mar 25, 2020

djc left a comment

djc Mar 25, 2020

duesee Mar 25, 2020 •

edited

Loading

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

djc Mar 25, 2020

duesee commented Mar 25, 2020

djc commented Mar 26, 2020

duesee commented Mar 27, 2020

Refactoring modules into parser:: and types:: + minor cleanups #80

Refactoring modules into parser:: and types:: + minor cleanups #80

Conversation

duesee commented Mar 25, 2020

duesee commented Mar 25, 2020

djc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duesee Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duesee commented Mar 25, 2020

djc commented Mar 26, 2020

duesee commented Mar 27, 2020

Refactoring modules into `parser::` and `types::` + minor cleanups #80

Refactoring modules into `parser::` and `types::` + minor cleanups #80

duesee Mar 25, 2020 •

edited

Loading