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

Consider extra empty space in BODYSTRUCTURE #271

Merged

Conversation

gaynetdinov
Copy link
Contributor

Hi! I'm working on (very old) project that's excessively using net-imap to access remote IMAP servers. Since ages we have some 'imap hacks' implemented on top of net-imap. These hacks were introduced to cope with some unusual responses from IMAP servers of our customers. I thought that I'd try to 'backport' some of them if you'd consider them viable/useful.

In this my first PR I'm 'backporting' one fix we had since 2016. Responses from some IMAP servers contained extra space like so ((\"text\" \"plain\" (\"charset\" \"UTF-8\"). The fix seems to be easy and should not do harm I guess. I'm not sure if the fix is idiomatic according to the recent changes in net-imap (we're bumping net-imap frim 0.3.7 to 0.4.10 right now, so how we implemented the fix long time ago might not be pretty in 0.4.10).

Thank you for your time!

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.

Since ages we have some 'imap hacks' implemented on top of net-imap. These hacks were introduced to cope with some unusual responses from IMAP servers of our customers. I thought that I'd try to 'backport' some of them if you'd consider them viable/useful.

That's great! My experience is very similar. :) And, I hope the jump from 0.3.7 to 0.4.10 isn't too difficult. As you've probably seen, some of the biggest parser changes were in the BODYSTRUCTURE code. And I have even more patches that I still haven't integrated into net-imap, yet!

I'm not sure if the fix is idiomatic according to the recent changes in net-imap

That's no problem. The updates are quite simple. I added a couple of suggestions. Thanks!

lib/net/imap/response_parser.rb Outdated Show resolved Hide resolved
@nevans
Copy link
Collaborator

nevans commented Apr 19, 2024

I'll add that I do have some concerns about simply skipping over extra spaces in general, because I have seen (or maybe I only misremembered seeing?) servers mistakenly send nothing rather than NIL, which converts " NIL " into " ". But since you've run this since 2016 with no noticeable issues (and it passes all other tests), I think it should be safe.

@gaynetdinov gaynetdinov requested a review from nevans April 23, 2024 13:52
@nevans nevans merged commit 119f43a into ruby:master Apr 23, 2024
13 checks passed
@gaynetdinov gaynetdinov deleted the consider-extra-empty-space-in-bodystructure branch April 23, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants