Expose Message::Field Unparsed Value

[Tensho]

I know this is not mandrill_dm fault mail gem doesn't expose public api to retrieve origin message field value, particularly merge vars. But my colleagues constantly argues on instance_variable_get(:@value) in specs to verify mail message correctness. So I propose to extend Mail::Field with required attribute reader.

Improvements

• Add appraisal gem to test against different dependency versions

Related PRs & Issues

• https://github.com/mikel/mail/issues/1195
• https://github.com/mikel/mail/pull/1196

[kshnurov]

instance_variable_defined? should be used instead of version number comparison. You should try making a PR to mail gem.

I'm not feeling comfortable with monkey-patching the whole library in a gem, even with such a simple code. @spovich what do you think?

[Tensho]

@kshnurov, I chose Mail::VERSION.version to emphasize breaking change between dependency versions. I used to find versions comparison over particular method or instance variable in many other open source libraries.

I'll make PR in case if the discussion there gives any feedback. It seems to me I saw controversial mail author opinion regarding OO purity on the subject long time ago, so I'm not sure Mail::Field will be changed here.

[kshnurov]

@Tensho it is a terrible practice, I'd avoid doing that whenever possible.

PR with a proposed change will have much more chances to be discussed and merged. Issue have a 0% merging chance ;)

[Tensho]

@kshnurov, As an engineer I used to discuss the problem and gather opinions upfront the development process ^_^ The most valuable code that was never written ^_~

I would really appreciate if you give me an example why dependency version comparison is a bad practice 🙇 I changed to #instance_variable_defined?, but would like to pay attention it makes check every call to #get_value in contrast to conditional method definition in previous variant.

[Tensho]

Created mikel/mail#1196.

[kshnurov]

@Tensho with instance_variable_defined? you're checking if a variable actually exists. With version number you're hoping that it exists and it hasn't changed in future versions. Code based on hope is a bad code :)

Version comparison is often used to check e.g. ActiveRecord major version and use appropriate code. Method's behavior may change and there's no other way to know about it rather that to check version number. If you can use respond_to?/method_defined? - use it, there's much less chance that it will break.

[Tensho]

Thank you for the sharing your thoughts 🙇 With the version number I explicitly describe the fact, that behavior changed between 2.6 and 2.7, instead of Mail::Field capabilities inspecting. Semantic versioning allows to reason about public and private API changes pretty well. I'm not hoping blindly on some code, but declare granularly dependencies version, which I believe should adhere any sustainable library.

I agree about "major" version checks, when we talk about public API, but mandrill_dm was compelled to use private part (instance variables within introspection Ruby capabilities) and it's OK to work with "minor" version check in such situation.

Of course, I'm still not any sustainable gem owner and may be wrong regarding the subject, but at least I tried to show up all arguments I have ^_^ Thank you for your patience 🙇

[Tensho]

https://github.com/mikel/mail/commit/7c22a8e9b5682d835f33a2f4e54bee409b944516 Exposed API was merged. I'll update the PR as soon as mail 2.7.1 is released at rubygems.

@kshnurov, would you like to have appraisal anyway in this gem?

[kshnurov]

@Tensho sure, why not.

[Tensho]

mail 2.7.1 was released yesterday. Updated the current branch with the commits squash. @kshnurov, @spovich Please let me know if I can do something else here 🙇

[Tensho]

@kshnurov Fixed

[Tensho]

@kshnurov Addressed all your concerns I think. Let me know if there's anything else you can think of 🙇

[kshnurov]

@Tensho sorry, forgot to hit "Finish review". Please clarify the comments and we're good to go!

[Tensho]

@kshnurov Fixed. I'm eager to squash commits before the merge.

[kshnurov]

@Tensho thanks!

[Tensho]

@kshnurov I'd expect squash review commits before merge ^_^ Anyway, thank you, Kirill 🙇 I'd really appreciate if you release this to rubygems as soon as possible 🙇

[kshnurov]

@Tensho sure. @spovich I've made a 1.3.6 release, could you please push it to Rubygems or give me access?

[Tensho]

@kshnurov @spovich Please could you tag with v1.3.6 the release commit either?

[kshnurov]

@kshnurov done

[Tensho]

Just a small note, I think it's reasonable to put the tag exactly on Release 1.3.6 or Bump to 1.3.6 commits. Anyway, thank you 🙇