Recognize Kind regards as a signature#71
Uh oh!
There was an error while loading. Please reload this page.
Conversation
mattyoho commented Mar 22, 2019
Hi, @askehansen! Thanks for submitting a pull request!
I completely agree this would be the best way to go about this and have been considering adding such an API, but haven't had the time to look into it seriously. I'm a little reticent to begin adding additional arbitrary signatures in the manner of this patch. Would you be interested in trying for a version of the user-configurable API you've described? I would help with review and with getting it merged and released if we could come up with something that felt right. Thanks! |
askehansen commented Mar 26, 2019
Yes I would :) |
askehansen commented Apr 3, 2019
@mattyoho I have extended the Regards are configured as an array that is empty by default. You would configure it like this: EmailReplyParser.configuredo |config| config.regards=['kind regards','best regards']endThe parser will now look for regards fragments and tag them as hidden, just like a signature fragment. It only does this if you have configured any regards. Let me know what you think :) |
mattyoho left a comment
mattyoho left a comment There was a problem hiding this comment.
@askehansen Thanks for taking another pass at this! I like the direction this is headed. 👍
I apologize for the delay in getting back to you on this PR.
I've left a few comments inline, but the essential point is that I think we should stick with the domain term signature and treat the configuration option as a way to customize what the library considers signature text.
Also, I've just enabled TravisCI builds for this repository in #72, so if you could merge in (or rebase on) master you'll enable the build for this branch.
Thanks!
| # A Configuration instance. | ||
| classConfiguration | ||
| # Configuration has an Array of regards | ||
| attr_accessor:regards |
There was a problem hiding this comment.
I think, at least generally in English, we'd refer to:
Warm regards,
Matt
as a signature as well, just a particular type of signature.
Rather than introduce the more-specific domain term regards, I think we should name this in terms of signature customization.
| require're2' | ||
| RE2::Regexp.new(value,case_sensitive: false) | ||
| rescueLoadError | ||
| Regexp.new(value,ignore_case: true) |
There was a problem hiding this comment.
Currently, we'll create a new Regexp object with each email message we parse, though the content of the regular expression won't be changing. I think we want to memoize it in one way or another.
| # Mark the current Fragment as a regards if regards are configured and | ||
| # the current line is empty and the Fragment starts with a common regards | ||
| # indicator. | ||
| ifregards_regex && @fragment && line == EMPTY |
There was a problem hiding this comment.
Not to keep repeating myself, but as discussed above I think we should fold this into the signature check.
But this does raise a question: should the signature customization be purely additive, or as a user of the library should I be able to clear out the default signature matchers? I'm inclined toward the latter.
2 participants
This pull request fixes#70
All I did was update the SIGNATURE regex to include multiple kind of "Kind regards", including one in danish:
https://github.com/github/email_reply_parser/compare/master...askehansen:kind-regards-signature?expand=1#diff-240c7196a90b5bff539340e9dbb77926R135
A better solution would be to keep these in a list and provide an interface for people to extend it in their codebase and locale.