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

Recognize Kind regards as a signature #71

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

Conversation

askehansen
Copy link

This pull request fixes #70

All I did was update the SIGNATURE regex to include multiple kind of "Kind regards", including one in danish:

Kind regards
Warm regards
Best regards
Med venlig hilsen

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.

@mattyoho
Copy link
Contributor

Hi, @askehansen! Thanks for submitting a pull request!

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.

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

Yes I would :)
Would it make sense to have preconfigured just a single item in the list? Or should it be empty by default?

@askehansen
Copy link
Author

@mattyoho I have extended the Fragment class so it can also be a regards instead of signature. I have also created a configuration class.

Regards are configured as an array that is empty by default. You would configure it like this:

EmailReplyParser.configure do |config|
  config.regards = ['kind regards', 'best regards']
end

The 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 :)

Copy link
Contributor

@mattyoho mattyoho left a comment

Choose a reason for hiding this comment

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

@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.
class Configuration
# Configuration has an Array of regards
attr_accessor :regards
Copy link
Contributor

Choose a reason for hiding this comment

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

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)
rescue LoadError
Regexp.new(value, ignore_case: true)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
if regards_regex && @fragment && line == EMPTY
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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.

Kind regards
2 participants