Skip to content
This repository was archived by the owner on Jul 22, 2019. It is now read-only.

[WIP] BODYSTRUCTURE FETCH response #16

Closed
wants to merge 1 commit into from
Closed

[WIP] BODYSTRUCTURE FETCH response #16

wants to merge 1 commit into from

Conversation

sanmai-NL
Copy link
Contributor

Publication of very early state, just to make reviewing and collaboration easier. The bodystructure test still fails.

test parser::tests::bodystructure ... FAILED

failures:

---- parser::tests::bodystructure stdout ----
        thread 'parser::tests::bodystructure' panicked at 'unexpected response Error(Alt)', src/parser.rs:640:23
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    parser::tests::bodystructure

@sanmai-NL sanmai-NL mentioned this pull request Dec 10, 2017
Open
@sanmai-NL
Copy link
Contributor Author

My last comment in the relevant issue, to be discussed here.

djc
djc suggested changes Dec 10, 2017
View reviewed changes
src/types.rs
@@ -97,6 +97,9 @@ pub enum AttributeValue<'a> {
index: Option<u32>,
data: Option<&'a [u8]>,
},
Bodystructure {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the surrounding style, single-element variants shouldn't be tuple variants.

src/parser.rs
@@ -621,6 +629,18 @@ mod tests {
}
}

#[test]
fn bodystructure() {
const RESPONSE: &[u8] = b"* 15 FETCH (BODYSTRUCTURE (\"TEXT\" \"PLAIN\" (\"CHARSET\" \"iso-8859-1\") NIL NIL \"QUOTED-PRINTABLE\" 1315 42 NIL NIL NIL NIL))\r\n";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure lines are no longer than 99 characters.

src/parser.rs
match parse_response(RESPONSE) {
IResult::Done(_, Response::Fetch(_, attrs)) => {
let body = &attrs[0];
assert!(if let &AttributeValue::Bodystructure { .. } = body { true } else { false }, "body = {:?}", body);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't very easy to grok. It's a fairly long expression with a sizable amount of complexity, all on a single line. It would probably be better to just assign the boolean to a local variable, then assert!() that.

@sanmai-NL
Copy link
Contributor Author

@djc: thanks for your comments. But note that this is not a submission for review, see opening post and WIP (work-in-progress) in title. I’m aware of the issues you mention. What would be most helpful is some guidance on how to write the combinators themselves. I’ll get back to it it soon again.

@djc
Copy link
Owner

djc commented Dec 11, 2017

Ah yes, sorry for the eager review. I think my comment from #15 should help decide how to write the combinators?

@sanmai-NL
Copy link
Contributor Author

Update: I haven’t spent more time on this yet. I’ve prioritized other dev work since. I’ll keep you informed about my progress!

@lberezy lberezy mentioned this pull request Apr 17, 2019
Closed
@sanmai-NL
Copy link
Contributor Author

I'm closing this PR, since I haven't been able to spend time on this for a while now.

@sanmai-NL sanmai-NL closed this Apr 17, 2019
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Reviewers

@djc djc djc requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sanmai-NL @djc