-
Notifications
You must be signed in to change notification settings - Fork 332
server: Add Mailbox.Close, make ListMailboxes return MailboxInfo #341
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #341 +/- ##
==========================================
+ Coverage 73.55% 73.58% +0.02%
==========================================
Files 32 32
Lines 3524 3528 +4
==========================================
+ Hits 2592 2596 +4
Misses 643 643
Partials 289 289
Continue to review full report at Codecov.
|
|
Honestly I'm not a big fan of this change. This doesn't fit well with the v1 interfaces. I'd prefer to start working on a v2 branch to address this issue (by adding a mandatory |
|
Alright, I will update this PR this weekend (hopefully, soju :\ ). |
|
I guess, https://github.com/foxcpp/go-imap-maildir will be built along with go-imap v2 then :) |
| } | ||
| defer mbox.Close() | ||
|
|
||
| return mbox.SetSubscribed(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be a User method instead, so that a mailbox is only opened on SELECT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I think I even mentioned that somewhere but this is out-of-scope of this PR.
|
Overall looks good. We could discuss/address the comment in another issue/PR if you prefer. |
emersion
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This change is backward compatible with v1.go-imap-unselect needs an update to avoid 'leaking' mailboxes, I propose to merge it into go-imap along the way per #322.
Closes #337.