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

server: Move to using Backend.Features() instead of Enable* flags #112

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

foxcpp
Copy link
Collaborator

@foxcpp foxcpp commented Jul 14, 2020

Closes #107.

@codecov
Copy link

codecov bot commented Jul 14, 2020

Codecov Report

Merging #112 into master will increase coverage by 0.38%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
conn.go 59.82% <57.14%> (+0.76%) ⬆️
server.go 57.27% <60.00%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1918b06...fd566a2. Read the comment docs.

@corny
Copy link

corny commented Jul 14, 2020

I would like to see a func (f Feature) Contains(feat int) bool { ... } method to clean up the code.

backend.go Outdated Show resolved Hide resolved
@emersion
Copy link
Owner

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.
@foxcpp
Copy link
Collaborator Author

foxcpp commented Jan 17, 2021

Addressed feedback: added Feature.Contains() and inverted flags (making extensions opt-in rather than opt-out).

Copy link
Owner

@emersion emersion left a 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...

backend.go Outdated Show resolved Hide resolved
backend.go Show resolved Hide resolved
backend.go Show resolved Hide resolved
@emersion emersion removed the breaking label May 2, 2021

const (
// SMTPUTF8 (RFC 6531) extension.
FeatureSMTPUTF8 Feature = 2 << iota
Copy link
Owner

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?

Copy link
Collaborator Author

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
Copy link
Owner

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Server.Enable* with backend Features() method
3 participants