Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

API changes and cleanups (obsoletes #80) #81

Closed
wants to merge 10 commits into from
Closed

API changes and cleanups (obsoletes #80) #81

wants to merge 10 commits into from

Conversation

duesee
Copy link
Contributor

@duesee duesee commented Mar 27, 2020

No description provided.

@duesee duesee mentioned this pull request Mar 27, 2020
Closed
djc
djc reviewed Mar 27, 2020
View reviewed changes
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Bunch more nits about the presentation here. Getting closer though! Thanks for sticking with it.

imap-proto/src/parser/core.rs
@@ -1,6 +1,4 @@
use nom::{self, IResult};

use std::str;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we've come to agreement on the removal of the std::str import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I forgot it. See last comment.

imap-proto/src/lib.rs Outdated

pub mod body;
pub mod body_structure;
// Public API
Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, but this still has too much going on. Don't do reformatting in the same commit as changing the public API, please, because it's hard for me to review.

imap-proto/src/parser/rfc3501/mod.rs Outdated
@@ -60,15 +64,15 @@ named!(mailbox<&str>, map!(

named!(flag_extension<&str>, map_res!(
recognize!(pair!(tag!("\\"), take_while!(atom_char))),
str::from_utf8
std::str::from_utf8
Copy link
Owner

Choose a reason for hiding this comment

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

Also these changes to add std before str belong in the other commit if we want them at all.

imap-proto/src/parser/rfc4551.rs Outdated Show resolved Hide resolved
imap-proto/src/parser/core.rs Show resolved Hide resolved
imap-proto/src/parser/rfc3501/mod.rs Outdated
@@ -534,7 +534,6 @@ pub fn parse_response(msg: &[u8]) -> ParseResult {
response(msg)
}


Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't belong in this commit. Maybe some earlier commit made changes here? Otherwise a separate commit for this is also fine.

@djc
Copy link
Owner

djc commented Mar 27, 2020

(Also feel free to use push -f to overwrite the previous commits on a PR branch, rather than opening a new one.)

@duesee
Copy link
Contributor Author

duesee commented Mar 27, 2020 •
edited
Loading

Okay, I further split the changing api commit. I am not sure why, but when I reverted the std::str change, I always ended up in conflicting merge hell. I hope the comment is sufficient.

@djc
Copy link
Owner

djc commented Mar 27, 2020

So I have taken 7 of your commits, applied minor fixes and pushed them to master. Please try to rebase this branch on master and clean up the commit history. I think I want the following commits:

  • Reformat imports everywhere (reformatting only, please)
  • Import the nom digit1 function directly
  • Move modules around in the hierarchy/filesystem
  • Move ParseResult
  • Add is_ prefix to predicates (remove the std::str addition from that commit)
  • Reorder parsers and add visual sections (remove the change to the other file)
  • Expand the core::* imports (don't forget to reformat after this)
  • Add is_char() and use it to fix is_atom_char() and is_text_char()
  • Replace literal with strict version

This really is highly similar to what you've got now, but there's still quite a bit of noise in there. Fixing up the std::str stuff in a later commit is too ugly for me in this case, sorry. :( You should be able to squash that commit into one of the earlier ones to fix up most of the issues.

@duesee
Copy link
Contributor Author

duesee commented Mar 28, 2020 •
edited
Loading

Sorry, just to be clear, is there anything left you do not want? Not by means of commit presentation, but the end result?

@djc
Copy link
Owner

djc commented Mar 28, 2020

I don't think there's anything I don't want here, but it'll be easier to be firm on that once I can review it more closely because it is in a more easily reviewable state.

@djc
Copy link
Owner

djc commented Mar 28, 2020

Current patches are looking great, would be happy to merge once you clean up the formatting!

(You might like this one-liner: git filter-branch --tree-filter 'cargo fmt' -- origin/master....)

@djc
Copy link
Owner

djc commented Mar 28, 2020

In fact, I figured I might as well do that for you -- this has been merged. Thanks!!

@djc djc closed this Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc left review comments

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@duesee @djc