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

fix: Add suffix to template files #362

Merged
merged 4 commits into from Nov 6, 2023
Merged

fix: Add suffix to template files #362

merged 4 commits into from Nov 6, 2023

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 14, 2023

Add .hbs as a suffix to all Handlebars template files.

This helps in:

  • understanding the purpose of files in this repo (especially when searching/grepping)
  • troubleshooting when something goes wrong
  • prevent spurious tooling warnings, e.g. json syntax errors reported by GitHub and highlighted in red in this screenshot:
    image

@rotu rotu marked this pull request as ready for review September 15, 2023 06:17
@rotu rotu requested a review from a team as a code owner September 15, 2023 06:17
@wraithgar wraithgar changed the title Add .handlebars suffix to template files fix: Add .handlebars suffix to template files Sep 15, 2023
@wraithgar
Copy link
Member

Look good other than the linting error now that extname isn't being used anymore.

@lukekarrys
Copy link
Member

Can it use .hbs instead of .handlebars? I generally prefer shorter file extensions if possible. I think it makes the file sidebar easier to scan visually:

Screenshot 2023-09-15 at 9 04 16 AM Screenshot 2023-09-15 at 9 04 40 AM

@rotu
Copy link
Contributor Author

rotu commented Sep 15, 2023

Can it use .hbs instead of .handlebars? I generally prefer shorter file extensions if possible.

I can do that.

BTW, when doing this PR, I realized that the existing .yml/.md/... extension is stripped off for purposes of assigning a name to the partial. I figure that's why pkg.json is named as such (instead of package.json, which would probably interfere with tooling).

Since that extension is ignored, maybe we should either:

  • Must name the files e.g. _step-audit.handlebars and add a comment at the top like {{! compiled to yaml }}
  • Make the destination format part of the name like _step-audit-yaml.handlebars and use {{> stepAuditYaml }}

Thoughts?

@wraithgar
Copy link
Member

ircc think it's called pkg.json so that npm doesn't pick it up and assume that subdirectory is a package. The presence of a file named package.json is very load bearing in npm.

@wraithgar
Copy link
Member

wraithgar commented Sep 15, 2023

Thoughts?

Having the eventual extension be in the filename already is VERY helpful imo, a yaml/etc comment just isn't the same. So _step-audit-yaml.handlebars would be just keen imo.

@rotu
Copy link
Contributor Author

rotu commented Sep 15, 2023

ircc think it's called pkg.json so that npm doesn't pick it up and assume that subdirectory is a package. The presence of a file named package.json is very load bearing in npm.

Exactly what I meant! Calling a file package.json has unintended consequences. But calling it pkg.json, package.json.handlebars, or package-json.handlebars has no such baggage.

So _step-audit-yaml.handlebars would be just keen imo.

Sorry, I don't speak jive. I think you mean that I should go with _step-audit-yaml.handebars (or, more likely, _step-audit-yaml.hbs). Please correct if wrong!

@wraithgar
Copy link
Member

wraithgar commented Sep 15, 2023

I think _step-audit-yaml.hbs is a good choice, it keeps the yaml in the filename while removing .yaml from the "extension" chain.

ETA: It also relieves us of the pkg.json package.json disconnect problem, we can have package-json.hbs. It's a win all around.

@rotu rotu changed the title fix: Add .handlebars suffix to template files fix: Add suffix to template files Sep 16, 2023
@wraithgar
Copy link
Member

lgtm, I'll let @lukekarrys land this one.

I really love being able to remove the local eslint config!

@lukekarrys lukekarrys merged commit 3e1792c into npm:main Nov 6, 2023
26 checks passed
@github-actions github-actions bot mentioned this pull request Nov 6, 2023
wraithgar pushed a commit that referenced this pull request Nov 6, 2023
This has always been true for this package, but we haven't made too many
API breaking changes. Recently #362 landed which is a breaking change,
since the API includes shadowing and using these files as partials. So
it is best to embrace these types of changes and document the behavior.
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.

None yet

3 participants