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

Allow any instead of custom *Author type #1630

Merged
merged 2 commits into from
Jan 2, 2023
Merged

Allow any instead of custom *Author type #1630

merged 2 commits into from
Jan 2, 2023

Conversation

meatballhat
Copy link
Member

@meatballhat meatballhat commented Dec 19, 2022

What type of PR is this?

  • cleanup

What this PR does / why we need it:

Drop the custom *Author type and allow any with the assumption that it either implements fmt.Stringer or can otherwise be safely Sprintf'd via %s.

Which issue(s) this PR fixes:

Supports #1586 given the *Author type was defined in app.go which is slated for removal.

@meatballhat meatballhat added this to the Release 3.x milestone Dec 19, 2022
@meatballhat meatballhat requested a review from a team as a code owner December 19, 2022 03:01
@meatballhat meatballhat mentioned this pull request Dec 19, 2022
asahasrabuddhe
asahasrabuddhe previously approved these changes Dec 19, 2022
Copy link
Member

@asahasrabuddhe asahasrabuddhe left a comment

Choose a reason for hiding this comment

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

This looks good. Why are the tests failing though?

@abitrolly
Copy link
Contributor

abitrolly commented Dec 19, 2022

@asahasrabuddhe looks like net/mail adds 500kB to the size of the package.

@meatballhat
Copy link
Member Author

@asahasrabuddhe @abitrolly I introduced this because I generally consider using an existing standard library type to be preferable to maintaining our own. Considering the increase in binary size, and that we're not going to be attempting to directly send email with the result of *mail.Address.String, I'm leaning towards not making this change. WDYT?

@abitrolly
Copy link
Contributor

@meatballhat I think like this issue needs to be addressed upstream somehow. Implementing interface for storing two strings should not result in 500kB increase. Why does this happen?

@meatballhat
Copy link
Member Author

@meatballhat I think like this issue needs to be addressed upstream somehow. Implementing interface for storing two strings should not result in 500kB increase. Why does this happen?

@abitrolly wow, yeah, I hadn't thought of it that way!

@asahasrabuddhe
Copy link
Member

This change does not make sense considering the increase in binary size. If we find a way like implementing an interface which is still compatible with the standard library and also does not affect the binary size, that's the way to go.

@dearchap
Copy link
Contributor

@meatballhat Just revert to old struct it was just 2 fields. Simple and direct or make an interface and add an example in docs of how to use mail.Address with it.

@meatballhat
Copy link
Member Author

@dearchap I think now that I'd like to use Authors []any given that the values are only ever passed to Sprintf-like functions with %s anyway. I'd like to keep reducing the surface area as much as possible, and making such a change would support that effort. WDYT?

@dearchap
Copy link
Contributor

dearchap commented Jan 1, 2023

@meatballhat That would be fine.

and updated tests to use both strings and *mail.Address
@meatballhat meatballhat changed the title Use *mail.Address instead of custom *Author type Allow any instead of custom *Author type Jan 1, 2023
@meatballhat meatballhat requested review from asahasrabuddhe and a team January 1, 2023 22:39
@meatballhat meatballhat merged commit 97854d2 into main Jan 2, 2023
@meatballhat meatballhat deleted the author-addr branch January 2, 2023 21:26
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.

4 participants