codeowners icon indicating copy to clipboard operation
codeowners copied to clipboard

feat: add matcher interface

Open haveachin opened this issue 2 years ago • 9 comments

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

* [email protected]

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

haveachin avatar Apr 25 '23 14:04 haveachin

Would appreciate this change as well

MB175 avatar May 24 '23 13:05 MB175

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 (https://github.com/hmarr/codeowners/issues/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.

hmarr avatar May 24 '23 19:05 hmarr

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 avatar Jun 13 '23 13:06 haveachin

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 avatar Jul 05 '23 08:07 haveachin

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

haveachin avatar Aug 29 '23 05:08 haveachin

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 https://github.com/hmarr/codeowners/pull/23 — have a quick look at let me know what you think.

hmarr avatar Nov 26 '23 19:11 hmarr

@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?

hmarr avatar Nov 29 '23 02:11 hmarr

LGTM

haveachin avatar Dec 01 '23 17:12 haveachin

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

itsteddyyo avatar Apr 22 '24 14:04 itsteddyyo

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

hmarr avatar Jun 25 '24 13:06 hmarr