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.

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

Add support for ENABLE (RFC 5161) #98

Closed
wants to merge 1 commit into from
Closed

Add support for ENABLE (RFC 5161) #98

wants to merge 1 commit into from

Conversation

arnt
Copy link
Contributor

@arnt arnt commented Jan 13, 2023

Hi,

this is almost a no-op because it doesn't change the parser.

Once this is merged, I'll contribute code to support UTF8=ACCEPT (RFC 6855), which depends on this, does extend the parser and has effect.

(There is a certain risk that I may implement QRESYNC as well.)

@nevans
Copy link
Collaborator

nevans commented Jan 13, 2023

Haha, coincidentally, I actually started to extract something like this from a much larger branch of mine, just last night. You may have saved me the effort of finishing that task. 😉

That other branch has massive other changes to ResponseParser, and I was delaying posting it until I've finished adding test coverage and benchmarks. I've fixed many many bugs, too... but I'm just not quite done with the tests and benchmarks. For what it's worth, that much larger branch also has a parser that can handle CONDSTORE and QRESYNC... and BINARY and ESEARCH and LIST-EXTENDED and UTF8=ACCEPT and several others. But I might not have the time to be comfortable with merging it or any significant part of it into HEAD for a few months. But I can do some basic tidy-up and post what I have. I'll try to do that by Monday, and maybe you can take a look at it... and maybe take it for a test drive, and use it as a basis for a more complete PR (I haven't written the command-side yet). :)

Copy link
Collaborator

@nevans nevans left a comment

Choose a reason for hiding this comment

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

Thanks again and sorry for the delay. I've made some minor stylistic and documentation tweaks, and I did pull in some changes from my forthcoming parser updates. Let me know what you think.

nevans added a commit that referenced this pull request Jan 22, 2023
Add support for ENABLE (RFC 5161)

Fixes #33.
@nevans
Copy link
Collaborator

nevans commented Jan 22, 2023

Ahh, I rebased before merging from the command line, so GitHub didn't close the PR as merged. ☹️ I thought it would consider the rebased a40c0cf to be functionally identical to this PR (as the git CLI does). Oops.

@nevans nevans closed this Jan 22, 2023
@nevans nevans reopened this Jan 22, 2023
@nevans nevans closed this Jan 22, 2023
@arnt
Copy link
Contributor Author

arnt commented Jan 22, 2023

Looks nice. Thanks.

You used double underscore in a couple of names, I assume that's a ruby convention with which I was not familiar. What does that mean?

@arnt arnt deleted the enable branch January 22, 2023 12:18
@nevans
Copy link
Collaborator

nevans commented Jan 23, 2023

You used double underscore in a couple of names, I assume that's a ruby convention with which I was not familiar. What does that mean?

It's not a ruby convention that I'm familiar with either. 😉 I'll add documentation to the code, but for now it's only documented in the commit message for 9caff31.

Methods are all named according to the pattern:

[
  modifier,
  grammar_rule.tr("-", "_"),
  subrule_or_modifier,
].compact.join("__")

I've been going through every rule in all of the IMAP RFCs' ABNF, documenting workarounds for common server bugs while fixing our own, etc. and this simplifies matching up the ABNF rules with the matching methods. So, eg: parens__objectid = "(" objectid ")" and nparens__objectid = "(" objectid ")" / nil or rules like mailbox-data might be subdivided into sub-rules such as mailbox_data__search and mailbox_data__status, etc.

@nevans nevans added the IMAP4rev2 Requirement for IMAP4rev2, RFC9051 label Feb 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IMAP4rev2 Requirement for IMAP4rev2, RFC9051
Development

Successfully merging this pull request may close these issues.

2 participants