-
Notifications
You must be signed in to change notification settings - Fork 481
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 a plugin for EmailAddress #694
base: master
Are you sure you want to change the base?
Conversation
FYI @KevinHock - thoughts? |
Hi @perryzjc, thank you for opening this PR. I noticed our PR checks did not run, so I'll have to figure what happened there first. Rest assured I'll get to this as soon as possible. |
@perryzjc hi again 😄 could you please merge master into your branch? |
@lorenzodb1 hi, I've merged the latest updates from the master branch into my feature branch as requested. Please let me know if there's anything else needed for this pull request. Thanks! |
@perryzjc looks like there's some pre-commit checks failing, please take a look |
51cfcee
to
7d4bc67
Compare
@lorenzodb1 Thanks for letting me know. I've resolved the issues and all checks are now passing in the workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we could add a way for someone to specify a domain so that this plugin flags only emails from that domain?
@lorenzodb1 What if we add an additional argument for the command line to specify the domains and make it the config in the baseline file?
So, instead of the usual: We'd have something like: The baseline file would then look like this:
This setup means our pre-commit checks could catch emails based on this configuration, giving us more control. I’m curious about your take on this. Are there any concerns regarding compatibility or implementation challenges we should be aware of? |
Hey @lorenzodb1 @jpdakran - just wanted to poke here as well. Hope @perryzjc contribution can be considered for merge. |
@perryzjc I just got around to review this and I wondering: do you still intend to add the |
This PR introduces a new feature in the form of an Email Address Detector.
Currently, the system does not have specific functionality that allows for the detection of email addresses.
What is the new behavior (if this is a feature change)?
With this feature change, the system can now detect email addresses. It uses a regex-based detector to identify valid email addresses and ignores certain whitelist email addresses to reduce false positives. It primarily validates the general format of email addresses but does not adhere strictly to email format standards such as RFC 5322 for the concerns of complexity and efficiency.
Does this PR introduce a breaking change?
This PR does not introduce a breaking change. The new detector class EmailAddressDetector is a standalone addition and does not alter or interfere with the existing code base.
In addition to the implementation of the Email Address Detector, this PR also includes comprehensive tests to ensure the correct functionality of the detector. It tests the detector against valid and invalid email addresses, and email addresses that are part of a larger string, as well as whitelist and non-whitelist email addresses.
The code documentation has been updated to reflect these changes. The new feature is expected to enhance the system's capabilities in handling email address-related data.