fix: update repo structure validation logic to disallow false positives #10
Conversation
chore: update README files and validation to reflect new requirements
cmd/readmevalidation/readmeFiles.go
Outdated
| // separateFrontmatter attempts to separate a README file's frontmatter content | ||
| // from the main README body, returning both values in that order. It does not | ||
| // validate whether the structure of the frontmatter is valid (i.e., that it's | ||
| // structured as YAML). |
cmd/readmevalidation/readmeFiles.go
Outdated
| const ( | ||
| // validationPhaseFileStructureValidation indicates when the entire Registry | ||
| // directory is being verified for having all files be placed in the file | ||
| // system as expected. | ||
| validationPhaseFileStructureValidation validationPhase = iota | ||
|
|
||
| // validationPhaseFileLoad indicates when README files are being read from | ||
| // the file system | ||
| validationPhaseFileLoad | ||
|
|
||
| // validationPhaseReadmeParsing indicates when a README's frontmatter is | ||
| // being parsed as YAML. This phase does not include YAML validation. | ||
| validationPhaseReadmeParsing | ||
|
|
||
| // validationPhaseReadmeValidation indicates when a README's frontmatter is | ||
| // being validated as proper YAML with expected keys. | ||
| validationPhaseReadmeValidation | ||
|
|
||
| // validationPhaseAssetCrossReference indicates when a README's frontmatter | ||
| // is having all its relative URLs be validated for whether they point to | ||
| // valid resources. | ||
| validationPhaseAssetCrossReference | ||
| ) |
There was a problem hiding this comment.
I know this is common, but I personally prefer to use a string type for validationPhase and forgo the iota. You then don't need to convert to string with .String() and instead can just do string(phase) when needed.
There was a problem hiding this comment.
Yeah, I think my main worry with it was that it felt like the string approach meant that the custom type couldn't have a valid/meaningful zero value. Like, if I use custom strings for the main options, it's still possible to make a zero value version of the field, even though it shouldn't exist. Iota at least makes it so that the zero value is still valid (and represents the base role)
There was a problem hiding this comment.
I'm going to go ahead with the current changes, but I'll bring it up in our 1:1. If you think it's still worth switching to strings, I can make a separate PR
| @@ -3,5 +3,5 @@ display_name: Nataindata | |||
| bio: Data engineer | |||
| github: nataindata | |||
| website: https://www.nataindata.com | |||
| status: community | |||
| status: partner | |||
Splitting up the main validation PR I've got cooking up, so that it's easier to review.
Changes made
/modulesdirectory, which the new checks did catch)cmddirectory structureextractFrontmatterintoseparateFrontmatter– now returns out the frontmatter, as well as the README bodylog.Paniccalls