refactor: apply majority of feedback

scripts/validate-contributor-readmes/main.go

@@ -53,10 +53,14 @@ type validationPhaseError struct {
 }
 
 func (vpe validationPhaseError) Error() string {
-	msg := fmt.Sprintf("Error during %q phase of README validation:", vpe.Phase)
+	validationStrs := []string{}
 	for _, e := range vpe.Errors {
-		msg += fmt.Sprintf("\n- %v", e)
+		validationStrs = append(validationStrs, fmt.Sprintf("- %v", e))
 	}
+	slices.Sort(validationStrs)
+
+	msg := fmt.Sprintf("Error during %q phase of README validation:", vpe.Phase)
+	msg += strings.Join(validationStrs, "\n")
 	msg += "\n"
 
 	return msg
@@ -96,124 +100,67 @@ func extractFrontmatter(readmeText string) (string, error) {
 	return fm, nil
 }
 
-// A validation function for verifying one specific aspect of a contributor's
+func validateContributorGithubUsername(githubUsername string) error {
-// frontmatter content. Each function should be able to return ALL data
+	if githubUsername == "" {
-// violations that apply to the function's area of concern, rather than
+		return errors.New("missing GitHub username")
-// returning the first error found
-type contributorValidationFunc = func(fm contributorFrontmatterWithFilePath) []error
-
-func validateContributorGithubUsername(fm contributorFrontmatterWithFilePath) []error {
-	problems := []error{}
-
-	if fm.GithubUsername == "" {
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: missing GitHub username",
-				fm.FilePath,
-			),
-		)
-		return problems
 	}
 
-	lower := strings.ToLower(fm.GithubUsername)
+	lower := strings.ToLower(githubUsername)
 	if uriSafe := url.PathEscape(lower); uriSafe != lower {
-		problems = append(
+		return fmt.Errorf("gitHub username %q is not a valid URL path segment", githubUsername)
-			problems,
-			fmt.Errorf(
-				"%q: gitHub username %q is not a valid URL path segment",
-				fm.FilePath,
-				fm.GithubUsername,
-			),
-		)
 	}
 
-	return problems
+	return nil
 }
 
-func validateContributorEmployerGithubUsername(fm contributorFrontmatterWithFilePath) []error {
+func validateContributorEmployerGithubUsername(
-	if fm.EmployerGithubUsername == nil {
+	employerGithubUsername *string,
+	githubUsername string,
+) []error {
+	if employerGithubUsername == nil {
 		return nil
 	}
 
 	problems := []error{}
-
+	if *employerGithubUsername == "" {
-	if *fm.EmployerGithubUsername == "" {
+		problems = append(problems, errors.New("company_github field is defined but has empty value"))
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: company_github field is defined but has empty value",
-				fm.FilePath,
-			),
-		)
 		return problems
 	}
 
-	lower := strings.ToLower(*fm.EmployerGithubUsername)
+	lower := strings.ToLower(*employerGithubUsername)
 	if uriSafe := url.PathEscape(lower); uriSafe != lower {
-		problems = append(
+		problems = append(problems, fmt.Errorf("gitHub company username %q is not a valid URL path segment", *employerGithubUsername))
-			problems,
-			fmt.Errorf(
-				"%q: gitHub company username %q is not a valid URL path segment",
-				fm.FilePath,
-				*fm.EmployerGithubUsername,
-			),
-		)
 	}
 
-	if *fm.EmployerGithubUsername == fm.GithubUsername {
+	if *employerGithubUsername == githubUsername {
-		problems = append(
+		problems = append(problems, fmt.Errorf("cannot list own GitHub name (%q) as employer", githubUsername))
-			problems,
-			fmt.Errorf(
-				"%q: cannot list own GitHub name (%q) as employer",
-				fm.FilePath,
-				fm.GithubUsername,
-			),
-		)
 	}
 
 	return problems
 }
 
-func validateContributorDisplayName(fm contributorFrontmatterWithFilePath) []error {
+func validateContributorDisplayName(displayName string) error {
-	problems := []error{}
+	if displayName == "" {
-	if fm.DisplayName == "" {
+		return fmt.Errorf("missing display_name")
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: GitHub user %q is missing display name",
-				fm.FilePath,
-				fm.GithubUsername,
-			),
-		)
 	}
 
-	return problems
-}
-
-func validateContributorLinkedinURL(fm contributorFrontmatterWithFilePath) []error {
-	if fm.LinkedinURL == nil {
 	return nil
 }
 
-	problems := []error{}
+func validateContributorLinkedinURL(linkedinURL *string) error {
-	if _, err := url.ParseRequestURI(*fm.LinkedinURL); err != nil {
+	if linkedinURL == nil {
-		problems = append(
+		return nil
-			problems,
-			fmt.Errorf(
-				"%q: linkedIn URL %q is not valid: %v",
-				*fm.LinkedinURL,
-				fm.FilePath,
-				err,
-			),
-		)
 	}
 
-	return problems
+	if _, err := url.ParseRequestURI(*linkedinURL); err != nil {
+		return fmt.Errorf("linkedIn URL %q is not valid: %v", *linkedinURL, err)
 	}
 
-func validateContributorEmail(fm contributorFrontmatterWithFilePath) []error {
+	return nil
-	if fm.SupportEmail == nil {
+}
+
+func validateContributorSupportEmail(email *string) []error {
+	if email == nil {
 		return nil
 	}
 
@@ -223,202 +170,131 @@ func validateContributorEmail(fm contributorFrontmatterWithFilePath) []error {
 	// an email, and especially with some contributors being individual
 	// developers, we don't want to do that on every single run of the CI
 	// pipeline. Best we can do is verify the general structure
-	username, server, ok := strings.Cut(*fm.SupportEmail, "@")
+	username, server, ok := strings.Cut(*email, "@")
 	if !ok {
-		problems = append(
+		problems = append(problems, fmt.Errorf("email address %q is missing @ symbol", *email))
-			problems,
-			fmt.Errorf(
-				"%q: email address %q is missing @ symbol",
-				fm.FilePath,
-				*fm.LinkedinURL,
-			),
-		)
 		return problems
 	}
 
 	if username == "" {
-		problems = append(
+		problems = append(problems, fmt.Errorf("email address %q is missing username", *email))
-			problems,
-			fmt.Errorf(
-				"%q: email address %q is missing username",
-				fm.FilePath,
-				*fm.LinkedinURL,
-			),
-		)
 	}
 
 	domain, tld, ok := strings.Cut(server, ".")
 	if !ok {
-		problems = append(
+		problems = append(problems, fmt.Errorf("email address %q is missing period for server segment", *email))
-			problems,
-			fmt.Errorf(
-				"%q: email address %q is missing period for server segment",
-				fm.FilePath,
-				*fm.LinkedinURL,
-			),
-		)
 		return problems
 	}
 
 	if domain == "" {
-		problems = append(
+		problems = append(problems, fmt.Errorf("email address %q is missing domain", *email))
-			problems,
-			fmt.Errorf(
-				"%q: email address %q is missing domain",
-				fm.FilePath,
-				*fm.LinkedinURL,
-			),
-		)
 	}
-
 	if tld == "" {
-		problems = append(
+		problems = append(problems, fmt.Errorf("email address %q is missing top-level domain", *email))
-			problems,
-			fmt.Errorf(
-				"%q: email address %q is missing top-level domain",
-				fm.FilePath,
-				*fm.LinkedinURL,
-			),
-		)
 	}
-
+	if strings.Contains(*email, "?") {
-	if strings.Contains(*fm.SupportEmail, "?") {
+		problems = append(problems, errors.New("email is not allowed to contain search parameters"))
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: email is not allowed to contain search parameters",
-				fm.FilePath,
-			),
-		)
 	}
 
 	return problems
 }
 
-func validateContributorWebsite(fm contributorFrontmatterWithFilePath) []error {
+func validateContributorWebsite(websiteURL *string) error {
-	if fm.WebsiteURL == nil {
+	if websiteURL == nil {
 		return nil
 	}
 
-	problems := []error{}
+	if _, err := url.ParseRequestURI(*websiteURL); err != nil {
-	if _, err := url.ParseRequestURI(*fm.WebsiteURL); err != nil {
+		return fmt.Errorf("linkedIn URL %q is not valid: %v", *websiteURL, err)
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: LinkedIn URL %q is not valid: %v",
-				fm.FilePath,
-				*fm.WebsiteURL,
-				err,
-			),
-		)
 	}
 
-	return problems
-}
-
-func validateContributorStatus(fm contributorFrontmatterWithFilePath) []error {
-	if fm.ContributorStatus == nil {
 	return nil
 }
 
-	problems := []error{}
+func validateContributorStatus(status *string) error {
+	if status == nil {
+		return nil
+	}
+
 	validStatuses := []string{"official", "partner", "community"}
-	if !slices.Contains(validStatuses, *fm.ContributorStatus) {
+	if !slices.Contains(validStatuses, *status) {
-		problems = append(
+		return fmt.Errorf("contributor status %q is not valid", *status)
-			problems,
-			fmt.Errorf(
-				"%q: contributor status %q is not valid",
-				fm.FilePath,
-				*fm.ContributorStatus,
-			),
-		)
 	}
 
-	return problems
+	return nil
 }
 
 // Can't validate the image actually leads to a valid resource in a pure
 // function, but can at least catch obvious problems
-func validateContributorAvatarURL(fm contributorFrontmatterWithFilePath) []error {
+func validateContributorAvatarURL(avatarURL *string) []error {
-	if fm.AvatarURL == nil {
+	if avatarURL == nil {
 		return nil
 	}
 
 	problems := []error{}
-	if *fm.AvatarURL == "" {
+	if *avatarURL == "" {
-		problems = append(
+		problems = append(problems, errors.New("avatar URL must be omitted or non-empty string"))
-			problems,
-			fmt.Errorf(
-				"%q: avatar URL must be omitted or non-empty string",
-				fm.FilePath,
-			),
-		)
 		return problems
 	}
 
 	// Have to use .Parse instead of .ParseRequestURI because this is the
 	// one field that's allowed to be a relative URL
-	if _, err := url.Parse(*fm.AvatarURL); err != nil {
+	if _, err := url.Parse(*avatarURL); err != nil {
-		problems = append(
+		problems = append(problems, fmt.Errorf("URL %q is not a valid relative or absolute URL", *avatarURL))
-			problems,
-			fmt.Errorf(
-				"%q: URL %q is not a valid relative or absolute URL",
-				fm.FilePath,
-				*fm.AvatarURL,
-			),
-		)
 	}
-
+	if strings.Contains(*avatarURL, "?") {
-	if strings.Contains(*fm.AvatarURL, "?") {
+		problems = append(problems, errors.New("avatar URL is not allowed to contain search parameters"))
-		problems = append(
-			problems,
-			fmt.Errorf(
-				"%q: avatar URL is not allowed to contain search parameters",
-				fm.FilePath,
-			),
-		)
 	}
 
 	supportedFileFormats := []string{".png", ".jpeg", ".jpg", ".gif", ".svg"}
 	matched := false
 	for _, ff := range supportedFileFormats {
-		matched = strings.HasSuffix(*fm.AvatarURL, ff)
+		matched = strings.HasSuffix(*avatarURL, ff)
 		if matched {
 			break
 		}
 	}
 	if !matched {
-		segments := strings.Split(*fm.AvatarURL, ".")
+		segments := strings.Split(*avatarURL, ".")
 		fileExtension := segments[len(segments)-1]
-		problems = append(
+		problems = append(problems, fmt.Errorf("avatar URL '.%s' does not end in a supported file format: [%s]", fileExtension, strings.Join(supportedFileFormats, ", ")))
-			problems,
-			fmt.Errorf(
-				"%q: avatar URL '.%s' does not end in a supported file format: [%s]",
-				fm.FilePath,
-				fileExtension,
-				strings.Join(supportedFileFormats, ", "),
-			),
-		)
 	}
 
 	return problems
 }
 
 func validateContributorYaml(yml contributorFrontmatterWithFilePath) []error {
-	validationFuncs := []contributorValidationFunc{
-		validateContributorGithubUsername,
-		validateContributorEmployerGithubUsername,
-		validateContributorDisplayName,
-		validateContributorLinkedinURL,
-		validateContributorEmail,
-		validateContributorWebsite,
-		validateContributorStatus,
-		validateContributorAvatarURL,
-	}
 	allProblems := []error{}
-	for _, fn := range validationFuncs {
+	addFilePath := func(err error) error {
-		allProblems = append(allProblems, fn(yml)...)
+		return fmt.Errorf("%q: %v", yml.FilePath, err)
 	}
+
+	if err := validateContributorGithubUsername(yml.GithubUsername); err != nil {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	if err := validateContributorDisplayName(yml.DisplayName); err != nil {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	if err := validateContributorLinkedinURL(yml.LinkedinURL); err != nil {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	if err := validateContributorWebsite(yml.WebsiteURL); err != nil {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	if err := validateContributorStatus(yml.ContributorStatus); err != nil {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+
+	for _, err := range validateContributorEmployerGithubUsername(yml.EmployerGithubUsername, yml.GithubUsername) {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	for _, err := range validateContributorSupportEmail(yml.SupportEmail) {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+	for _, err := range validateContributorAvatarURL(yml.AvatarURL) {
+		allProblems = append(allProblems, addFilePath(err))
+	}
+
 	return allProblems
 }
 
@@ -454,15 +330,7 @@ func parseContributorFiles(readmeEntries []readme) (
 		}
 
 		if prev, isConflict := frontmatterByUsername[processed.GithubUsername]; isConflict {
-			yamlParsingErrors.Errors = append(
+			yamlParsingErrors.Errors = append(yamlParsingErrors.Errors, fmt.Errorf("%q: GitHub name %s conflicts with field defined in %q", processed.FilePath, processed.GithubUsername, prev.FilePath))
-				yamlParsingErrors.Errors,
-				fmt.Errorf(
-					"%q: GitHub name %s conflicts with field defined in %q",
-					processed.FilePath,
-					processed.GithubUsername,
-					prev.FilePath,
-				),
-			)
 			continue
 		}
 
@@ -497,15 +365,7 @@ func parseContributorFiles(readmeEntries []readme) (
 		if _, found := frontmatterByUsername[companyName]; found {
 			continue
 		}
-		yamlValidationErrors.Errors = append(
+		yamlValidationErrors.Errors = append(yamlValidationErrors.Errors, fmt.Errorf("company %q does not exist in %q directory but is referenced by these profiles: [%s]", companyName, rootRegistryPath, strings.Join(group, ", ")))
-			yamlValidationErrors.Errors,
-			fmt.Errorf(
-				"company %q does not exist in %q directory but is referenced by these profiles: [%s]",
-				companyName,
-				rootRegistryPath,
-				strings.Join(group, ", "),
-			),
-		)
 	}
 	if len(yamlValidationErrors.Errors) != 0 {
 		return nil, yamlValidationErrors
@@ -525,13 +385,7 @@ func aggregateContributorReadmeFiles() ([]readme, error) {
 	for _, e := range dirEntries {
 		dirPath := path.Join(rootRegistryPath, e.Name())
 		if !e.IsDir() {
-			problems = append(
+			problems = append(problems, fmt.Errorf("detected non-directory file %q at base of main Registry directory", dirPath))
-				problems,
-				fmt.Errorf(
-					"Detected non-directory file %q at base of main Registry directory",
-					dirPath,
-				),
-			)
 			continue
 		}
 
@@ -565,6 +419,8 @@ func validateRelativeUrls(
 	problems := []error{}
 
 	for _, con := range contributors {
+		// If the avatar URL is missing, we'll just assume that the Registry
+		// site build step will take care of filling in the data properly
 		if con.AvatarURL == nil {
 			continue
 		}
@@ -574,13 +430,7 @@ func validateRelativeUrls(
 		}
 
 		if strings.HasPrefix(*con.AvatarURL, "..") {
-			problems = append(
+			problems = append(problems, fmt.Errorf("%q: relative avatar URLs cannot be placed outside a user's namespaced directory", con.FilePath))
-				problems,
-				fmt.Errorf(
-					"%q: relative avatar URLs cannot be placed outside a user's namespaced directory",
-					con.FilePath,
-				),
-			)
 			continue
 		}
 
@@ -588,14 +438,7 @@ func validateRelativeUrls(
 			*con.AvatarURL
 		_, err := os.ReadFile(absolutePath)
 		if err != nil {
-			problems = append(
+			problems = append(problems, fmt.Errorf("%q: relative avatar path %q does not point to image in file system", con.FilePath, *con.AvatarURL))
-				problems,
-				fmt.Errorf(
-					"%q: relative avatar path %q does not point to image in file system",
-					con.FilePath,
-					*con.AvatarURL,
-				),
-			)
 		}
 	}