Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate: refactor internal methods to use fs.FS interface for file handling #413

Merged
merged 14 commits into from
Oct 31, 2024

Conversation

SBGoods
Copy link
Contributor

@SBGoods SBGoods commented Oct 22, 2024

Previously, the internal methods and checks for the validate command were reading files/directories from file paths, which made internal unit testing difficult as it required static files and directories for each test case. This made it difficult to test new schema concepts with confidence. This PR refactors the validateStaticDocs() and validateLegacyWebsite() to use a fs.FS interface for filesystem operations, and refactors the tests to use a fstest.MapFS as an in-memory filesystem along with additional integration tests for the various validation checks.

There were several bugs that came up during testing which have fixes in this PR:

  • File extension check was only being run on index.md files instead of index.* files.
  • File extension check was outputting the incorrect valid extensions it's error message.
  • Frontmatter checks were not being run with the correct options on legacy index files.

@SBGoods SBGoods requested a review from a team as a code owner October 22, 2024 20:31
@austinvalle austinvalle added this to the v0.20.0 milestone Oct 30, 2024
Copy link
Member

@austinvalle austinvalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @SBGoods! Makes our lives much easier moving forward 👍🏻

I had one question about the hardcoded slashes, but everything else looks great! 🚀

result = errors.Join(result, err)

}
if dirExists(filepath.Join(v.providerDir, filepath.Join("website", "docs"))) {
if dirExists(v.providerFS, "website/docs") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify my understanding, removing all the filepath.Join and replacing with hardcoded forward slashes is because we are always going against the fs.FS abstraction now right? So we don't need to worry about OS specific paths.

Should these just all be using the path package?

Copy link
Contributor Author

@SBGoods SBGoods Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as far as the internal usages go, we should be using forward slashes instead of filepath.Join. For the error messages though, whenever we print out the path, we convert the slashes back to the OS separator so that it isn't confusing to Windows developers. So test assertions for error messages should still use filepath.Join (this is also why the testscript tests are skipped on Windows).

@SBGoods SBGoods merged commit 6c67ef2 into main Oct 31, 2024
6 checks passed
@SBGoods SBGoods deleted the SBGoods/refactor-validate branch October 31, 2024 13:41
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants