hmarr / codeowners

🔒 Command line tool and Go library for CODEOWNERS files
MIT License
167 stars 19 forks source link

feat: add matcher interface #19

Closed haveachin closed 2 months ago

haveachin commented 1 year ago

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

* haveachin@mail.localhost

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 'haveachin@mail.localhost' 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 commented 1 year ago

Would appreciate this change as well

hmarr commented 1 year ago

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.

haveachin commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

hmarr commented 9 months ago

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 commented 9 months ago

@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 commented 9 months ago

LGTM

itsteddyyo commented 4 months ago

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

hmarr commented 2 months ago

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