-
Notifications
You must be signed in to change notification settings - Fork 220
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
server: Move to using Backend.Features() instead of Enable* flags #112
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #112 +/- ##
==========================================
+ Coverage 62.09% 62.47% +0.38%
==========================================
Files 8 8
Lines 1108 1114 +6
==========================================
+ Hits 688 696 +8
+ Misses 312 311 -1
+ Partials 108 107 -1
Continue to review full report at Codecov.
|
I would like to see a |
I wonder how we should approach backwards compatibility. If we add a new feature, we won't be able to enable it by default without breaking the backend? |
Make all extensions opt-in rather than opt-out to make future additions easier and more consistent. Unrelated string(rune(char)) change is to make Go 1.15.6 'go test' happy.
Addressed feedback: added Feature.Contains() and inverted flags (making extensions opt-in rather than opt-out). |
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.
This now ensures adding new features to go-smtp won't break any backend, but it's not very ergonomic for users...
|
||
const ( | ||
// SMTPUTF8 (RFC 6531) extension. | ||
FeatureSMTPUTF8 Feature = 2 << iota |
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.
Why is this 2 instead of 1?
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.
Ouch, most likely a typo
// SMTPUTF8 (RFC 6531) extension. | ||
FeatureSMTPUTF8 Feature = 2 << iota | ||
// BINARYMIME (RFC 3030) extension. CHUNKING extension is always supported. | ||
FeatureBINARYMIME |
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.
The rest of the code seems to use the Go capitalization: "BinaryMIME"
(Same for "RequireTLS" I suppose)
Closes #107.