From d1c885d0e41c34c9890540c4942d58bf5c1067a9 Mon Sep 17 00:00:00 2001 From: Saswata Mukherjee Date: Fri, 4 Jun 2021 18:18:39 +0530 Subject: [PATCH] Implement suggestions Signed-off-by: Saswata Mukherjee --- main.go | 2 +- pkg/extflag/pathorcontent.go | 8 +-- pkg/mdformatter/linktransformer/link.go | 22 +------- pkg/mdformatter/linktransformer/link_test.go | 2 +- pkg/mdformatter/linktransformer/validator.go | 57 ++++++++++++-------- 5 files changed, 42 insertions(+), 49 deletions(-) diff --git a/main.go b/main.go index 606de27..c39eb4e 100644 --- a/main.go +++ b/main.go @@ -127,7 +127,7 @@ This directive runs executable with arguments and put its stderr and stdout outp "Absolute path links will be converted to relative links to anchor dir as well.").Regexp() // TODO(bwplotka): Add cache in file? linksValidateEnabled := cmd.Flag("links.validate", "If true, all links will be validated").Short('l').Bool() - linksValidateConfig := extflag.RegisterPathOrContent(cmd, "links.validate.config", "YAML file for skipping link check, with spec defined in github.com/bwplotka/mdox/pkg/linktransformer.Config", extflag.WithEnvSubstitution(true)) + linksValidateConfig := extflag.RegisterPathOrContent(cmd, "links.validate.config", "YAML file for skipping link check, with spec defined in github.com/bwplotka/mdox/pkg/linktransformer.Config", extflag.WithEnvSubstitution()) cmd.Run(func(ctx context.Context, logger log.Logger) (err error) { var opts []mdformatter.Option diff --git a/pkg/extflag/pathorcontent.go b/pkg/extflag/pathorcontent.go index 79fff3f..9a6c4ff 100644 --- a/pkg/extflag/pathorcontent.go +++ b/pkg/extflag/pathorcontent.go @@ -96,16 +96,16 @@ func (p *PathOrContent) Content() ([]byte, error) { } // WithRequired allows you to override default required option. -func WithRequired(r bool) Option { +func WithRequired() Option { return func(p *PathOrContent) { - p.required = r + p.required = true } } // WithRequired allows you to override default envSubstitution option. -func WithEnvSubstitution(e bool) Option { +func WithEnvSubstitution() Option { return func(p *PathOrContent) { - p.envSubstitution = e + p.envSubstitution = true } } diff --git a/pkg/mdformatter/linktransformer/link.go b/pkg/mdformatter/linktransformer/link.go index 2c1475d..acce8c7 100644 --- a/pkg/mdformatter/linktransformer/link.go +++ b/pkg/mdformatter/linktransformer/link.go @@ -253,31 +253,11 @@ func (v *validator) visit(filepath string, dest string) { } validator := v.validateConfig.GetValidatorForURL(dest) if validator != nil { - matched, err := validator.IsValid(dest) + matched, err := validator.IsValid(k, v) if matched && err == nil { return } } - - // Result will be in future. - v.destFutures[k].resultFn = func() error { return v.remoteLinks[dest] } - v.rMu.RLock() - if _, ok := v.remoteLinks[dest]; ok { - v.rMu.RUnlock() - return - } - v.rMu.RUnlock() - - v.rMu.Lock() - defer v.rMu.Unlock() - // We need to check again here to avoid race. - if _, ok := v.remoteLinks[dest]; ok { - return - } - - if err := v.c.Visit(dest); err != nil { - v.remoteLinks[dest] = errors.Wrapf(err, "remote link %v", dest) - } } type localLinksCache map[string]*[]string diff --git a/pkg/mdformatter/linktransformer/link_test.go b/pkg/mdformatter/linktransformer/link_test.go index d15b970..880e969 100644 --- a/pkg/mdformatter/linktransformer/link_test.go +++ b/pkg/mdformatter/linktransformer/link_test.go @@ -239,7 +239,7 @@ func TestValidator_TransformDestination(t *testing.T) { testutil.Equals(t, 0, len(diff), diff.String()) _, err = mdformatter.IsFormatted(context.TODO(), logger, []string{testFile}, mdformatter.WithLinkTransformer( - MustNewValidator(logger, []byte("version: 1\n\nvalidators:\n - regex: 'github'\n type: 'roundtrip'\n"), anchorDir), + MustNewValidator(logger, []byte("version: 1\n\nvalidators:\n - regex: 'bwplotka'\n type: 'ignore'\n"), anchorDir), )) testutil.Ok(t, err) }) diff --git a/pkg/mdformatter/linktransformer/validator.go b/pkg/mdformatter/linktransformer/validator.go index a7cad7c..1000813 100644 --- a/pkg/mdformatter/linktransformer/validator.go +++ b/pkg/mdformatter/linktransformer/validator.go @@ -6,18 +6,20 @@ package linktransformer import ( "strconv" "strings" + + "github.com/pkg/errors" ) type Validator interface { - IsValid(URL string) (bool, error) + IsValid(k futureKey, r *validator) (bool, error) } // GitHubValidator.IsValid skips visiting all github issue/PR links. -func (v GitHubValidator) IsValid(URL string) (bool, error) { +func (v GitHubValidator) IsValid(k futureKey, r *validator) (bool, error) { // Find rightmost index of match i.e, where regex match ends. // This will be where issue/PR number starts. Split incase of section link and convert to int. - rightmostIndex := v._regex.FindStringIndex(URL) - stringNum := strings.Split(URL[rightmostIndex[1]:], "#") + rightmostIndex := v._regex.FindStringIndex(k.dest) + stringNum := strings.Split(k.dest[rightmostIndex[1]:], "#") num, err := strconv.Atoi(stringNum[0]) if err != nil { return false, err @@ -29,48 +31,59 @@ func (v GitHubValidator) IsValid(URL string) (bool, error) { return false, nil } -// RoundTripValidator.IsValid returns false if url matches, to ensure it is visited by colly. -func (v RoundTripValidator) IsValid(URL string) (bool, error) { - return false, nil +// RoundTripValidator.IsValid returns true if url is checked by colly. +func (v RoundTripValidator) IsValid(k futureKey, r *validator) (bool, error) { + // Result will be in future. + r.destFutures[k].resultFn = func() error { return r.remoteLinks[k.dest] } + r.rMu.RLock() + if _, ok := r.remoteLinks[k.dest]; ok { + r.rMu.RUnlock() + return true, nil + } + r.rMu.RUnlock() + + r.rMu.Lock() + defer r.rMu.Unlock() + // We need to check again here to avoid race. + if _, ok := r.remoteLinks[k.dest]; ok { + return true, nil + } + + if err := r.c.Visit(k.dest); err != nil { + r.remoteLinks[k.dest] = errors.Wrapf(err, "remote link %v", k.dest) + return false, nil + } + return true, nil } // IgnoreValidator.IsValid returns true if matched so that link in not checked. -func (v IgnoreValidator) IsValid(URL string) (bool, error) { +func (v IgnoreValidator) IsValid(k futureKey, r *validator) (bool, error) { return true, nil } // GetValidatorForURL returns correct Validator by matching URL. func (v Config) GetValidatorForURL(URL string) Validator { - var u Validator for _, val := range v.Validators { switch val.Type { case roundtripValidator: if !val.rtValidator._regex.MatchString(URL) { continue } - u = val.rtValidator - return u + return val.rtValidator case githubValidator: if !val.ghValidator._regex.MatchString(URL) { continue } - u = val.ghValidator - return u + return val.ghValidator case ignoreValidator: if !val.igValidator._regex.MatchString(URL) { continue } - u = val.igValidator - return u + return val.igValidator default: - continue + panic("unexpected validator type") } } - // By default all links are ignored. - u = IgnoreValidator{} // No config file passed, so all links must be checked. - if len(v.Validators) == 0 { - u = nil - } - return u + return RoundTripValidator{} }