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

feat: add matcher interface #19

Merged
merged 3 commits into from
Jun 25, 2024
Merged

feat: add matcher interface #19

merged 3 commits into from
Jun 25, 2024

Conversation

haveachin
Copy link

Hey,

I was playing around with this library when I noticed that the regexp for emails was very restrictive. My setup looks something like this:

CODEOWNERS

main.go

package main

import (
	"log"
	"os"

	"github.com/hmarr/codeowners"
)

func main() {
	f, err := os.Open("CODEOWNERS")
	if err != nil {
		log.Fatal(err)
	}

	_, err = codeowners.ParseFile(f)
	if err != nil {
		log.Fatal(err)
	}
}

I was getting this error:

line 1: invalid owner format '[email protected]' at position 3

Here is a playground link if you want to try it yourself: https://play.golang.com/p/rxZi4JJA9JT

I swapped the current email regexp with a less restrictive one. This is the same regexp that is implemented in the HTML validation of an email input field: https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

@MB175
Copy link

MB175 commented May 24, 2023

Would appreciate this change as well

@hmarr
Copy link
Owner

hmarr commented May 24, 2023

Thanks for the PR!

The regex used in this library matches the one used by GitHub, so while it's not ideal, I think I'd rather keep the default aligned with the validation GitHub uses.

If we can make the parsing configurable, I would be open to making this available as an option (either a flag for the more permissive regex, or the ability to provide a custom email regex). The same applies to supporting GitLab syntax (#13) — while I think the default should align to GitHub's implementation as it's the most widespread, I'd be up for supporting more variants.

@haveachin
Copy link
Author

Hey, I added a Matcher interface to make the parsing extendable. There are no breaking changes to the API and the Matchers are totally optional. The extended API would look something like this:

var lessRestrictiveEmailRegexp = regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+\\/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$")

func MatchLessRestrictiveEmail(s string) (codeowners.Owner, error) {
	match := lessRestrictiveEmailRegexp.FindStringSubmatch(s)
	if match == nil {
		return codeowners.Owner{}, codeowners.ErrNoMatch
	}

	return codeowners.Owner{
		Value: match[0],
		Type:  codeowners.EmailOwner,
	}, nil
}

var lessRestrictiveEmailMatcher = codeowners.MatcherFunc(MatchLessRestrictiveEmail)

func main() {
	f, err := os.Open("CODEOWNERS")
	if err != nil {
		log.Fatal(err)
	}

	_, err = codeowners.ParseFile(f, lessRestrictiveEmailMatcher)
	if err != nil {
		log.Fatal(err)
	}
}

You could also just extend the default matcher:

func main() {
	f, err := os.Open("CODEOWNERS")
	if err != nil {
		log.Fatal(err)
	}

	_, err = codeowners.ParseFile(f, append(codeowners.DefaultMatchers, lessRestrictiveEmailMatcher)...)
	if err != nil {
		log.Fatal(err)
	}
}

@haveachin haveachin changed the title fix: email address regexp feat: add matcher interface Jun 13, 2023
@haveachin
Copy link
Author

Hey, could this please get another round of review. I'm looking forward to it being merged. If something doesn't meet project standards, just let me know.

@haveachin
Copy link
Author

@hmarr I would highly appreciate it if you could take a look at this PR

@hmarr
Copy link
Owner

hmarr commented Nov 26, 2023

Hey @haveachin, sorry for taking so long to look at this. Thanks for the work on this—overall it looks great!

I pushed a follow-up commit in #23 — have a quick look at let me know what you think.

@hmarr
Copy link
Owner

hmarr commented Nov 29, 2023

@haveachin my latest commit is included in this PR now—would you mind having a look and checking it meets your needs before I merge?

@haveachin
Copy link
Author

LGTM

@itsteddyyo
Copy link

Would be great if this could be merged 😄 Thanks to everyone involved for their work 😄

This was referenced May 29, 2024
@hmarr hmarr merged commit 075bbf2 into hmarr:main Jun 25, 2024
4 checks passed
@hmarr
Copy link
Owner

hmarr commented Jun 25, 2024

Sorry for taking so long to merge this, thanks for the contribution @haveachin!

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.

4 participants