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

Feat/template #10

Merged
merged 30 commits into from
Dec 20, 2022
Merged

Feat/template #10

merged 30 commits into from
Dec 20, 2022

Conversation

Si3rr4wow
Copy link
Collaborator

@Si3rr4wow Si3rr4wow commented Nov 24, 2022

This PR:

  • Adds a template to spool up a new patio
  • Builds a YAML template from the maintained JSON template and writes both to the bin
  • Validates the built templates pre-push
  • Replaces the openapi stub file created by feat/wizard with a copy of the relevant template

@Si3rr4wow Si3rr4wow changed the base branch from main to feat/wizard November 24, 2022 12:17
package.json Outdated
"dev": "node-dev src/index.ts",
"dev": "node-dev src/write-yaml-template.ts",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to be removed in a later commit, I just didn't wanna have to run the wizard over and over to test this

package.json Outdated Show resolved Hide resolved
Comment on lines 5 to 18
const walk = (dir: string, prevResults?: string[]): string[] => {
const list = fs.readdirSync(dir)
const results = prevResults || []
list.forEach((file) => {
file = path.resolve(dir, file)
const stat = fs.statSync(file)
if (stat && stat.isDirectory()) {
walk(file, results)
} else {
results.push(file)
}
})
return results
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I sense this would be more testable if I removed the dependency on fs

Copy link
Contributor

@robmorgan-tab robmorgan-tab Nov 24, 2022

Choose a reason for hiding this comment

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

Or you could use one of the many libraries which have this functionality built in. ;)

Then you have no need to write tests for the file system functionality - trust the library :D

Base automatically changed from feat/wizard to main November 24, 2022 15:16
package.json Outdated Show resolved Hide resolved
}

const makeBinTemplateSubdirectory = (jsonPath: string): void => {
const { jsonBinPath, yamlBinPath } = getBinTargetPaths(jsonPath)
Copy link
Contributor

@robmorgan-tab robmorgan-tab Nov 30, 2022

Choose a reason for hiding this comment

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

You could reduce duplication here

const createSubdirectory = (directoryPath: string): void => {
  const directoryName = path.dirname(directoryPath)
  const directoryStats = fs.statSync(directoryName, { throwIfNoEntry: false })
  if (!directoryStats?.isDirectory()) {
    fs.mkdirSync(directoryName)
  }
}

const makeBinTemplateSubdirectories = (jsonPath: string): void => {
  const { jsonBinPath, yamlBinPath } = getBinTargetPaths(jsonPath)
  [jsonBinPath, yamlBinPath].forEach((binPath) => createSubdirectory(binPath))
}

Copy link
Collaborator Author

@Si3rr4wow Si3rr4wow Dec 2, 2022

Choose a reason for hiding this comment

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

So this makeBinTemplateSubdirectories function is one of the things that disappeared one I started refactoring in #15. Turns out what I actually wanted was some conditions around a mkdir so I can use the same function to make the top level bin/templates and the subdirectories.

The forEach has given me the idea for a func that invokes getTemplateFileStrings and getBinTargetPaths and returns an array like { path: string, file: string }[], that should have better handing in writeFileToBin, like

getWritables(jsonPath).forEach(({ path, file }) => {
      makeDirectory(path.dirname(path))
      fileSystem.writeFileSync(path, file)
})

@robmorgan-tab
Copy link
Contributor

Looking really good.
My only thought/suggestion is that when running lay-patio the template directories & OpenAPI root file might be better if placed into a src directory.
That'd avoid the top level getting cluttered with additional tooling as the out of the box project grows.

@Si3rr4wow Si3rr4wow marked this pull request as ready for review December 20, 2022 15:17
@robmorgan-tab robmorgan-tab merged commit 74fc90a into main Dec 20, 2022
@robmorgan-tab robmorgan-tab deleted the feat/template branch December 20, 2022 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants