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

Add back method which was removed in #3969 #4396

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

bdukes
Copy link
Contributor

@bdukes bdukes commented Jan 4, 2021

Summary

The public method Mail.ConvertToText was accidentally removed in #3969, this PR adds it back.

Copy link
Contributor

@mitchelsellers mitchelsellers left a comment

Choose a reason for hiding this comment

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

All for this because it shouldn’t have been removed.

But going forward. Should we commit to having this method?

@bdukes
Copy link
Contributor Author

bdukes commented Jan 4, 2021

I think it's a helpful method (we discovered it was missing because @donker was using it). However, we are duplicating the functionality (#3969 moved this to be a private method of CoreMailProvider). It would be nice to adjust one of the call sites to use the other (which, in this case, would probably mean adjusting CoreMailProvider to call Mail.ConvertToText).

This isn't functionality I would plan to directly use, so I can't comment on whether it's still a fit solution, so perhaps there's an alternative we should promote instead.

@david-poindexter david-poindexter added this to the 9.8.1 milestone Jan 4, 2021
@david-poindexter
Copy link
Contributor

@bdukes shouldn't this be targeted to the 9.8.1 release branch?

@bdukes
Copy link
Contributor Author

bdukes commented Jan 4, 2021

Not necessarily. We can decide that's what we want to do. However, this is not a regression introduced by 9.8.1 (it was introduced in 9.8.0), so it doesn't meet the standard criteria for hot-fix adding to an active RC.

@david-poindexter david-poindexter removed this from the 9.8.1 milestone Jan 4, 2021
Copy link
Contributor

@donker donker left a comment

Choose a reason for hiding this comment

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

Thanks!

@bdukes bdukes added this to the 9.9.0 milestone Jan 6, 2021
@bdukes bdukes merged commit 2e19fbf into dnnsoftware:develop Jan 8, 2021
@bdukes bdukes deleted the restore-mail-convert-to-text branch January 8, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants