-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add template utils #534
Add template utils #534
Conversation
@@ -118,6 +118,49 @@ const PROJECT_TEMPLATE_TYPES = { | |||
}, | |||
}; | |||
|
|||
const TEMPLATE_TYPES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily a blocker, but it would be nice if this was something that the BE could return to us so that we could keep a single source of truth. I don't imagine that it will change frequently, but it'll be yet another area where we have to manually keep data in sync.
packages/cli-lib/templates.js
Outdated
@@ -27,6 +31,16 @@ const getFileAnnotations = filePath => { | |||
} | |||
}; | |||
|
|||
const getAnnotationsFromSource = source => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very similar to getFileAnnotations
. Could you update getFileAnnotations
to use this new getAnnotationsFromSource
func? Just to DRY things up a little.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, maybe getFileAnnotations
can use getAnnotationsFromSource
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good call. Will update
packages/cli-lib/templates.js
Outdated
* Returns true if: | ||
* filename is module.html, inside of *.module folder | ||
*/ | ||
const isModuleHTMLFile = filePath => MODULE_HTML_EXTENSION_REGEX.test(filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these module utils live in the templates.js
util file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good point
const annotations = getFileAnnotations(file); | ||
let data; | ||
try { | ||
data = fs.readFileSync(file, 'utf8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moves the file reading step from cli-lib to cli which I don't think has any negative impact
const annotation = match && match[1] ? match[1] : ''; | ||
return annotationKey => getAnnotationValue(annotation, annotationKey); | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as expected here https://github.com/HubSpot/hubspot-cms-vscode/pull/117/files#diff-dfcd8afcb372a75a1eb1bdc0af57d36a5e0a727dbca99350779abb21333a298fR65
and also tested the marketplace-validate
command out and that works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! This pattern looks cleaner 🙌
packages/cli-lib/modules.js
Outdated
@@ -4,6 +4,11 @@ const { getCwd, getExt, splitHubSpotPath, splitLocalPath } = require('./path'); | |||
const { walk } = require('./lib/walk'); | |||
const { MODULE_EXTENSION } = require('./lib/constants'); | |||
|
|||
// Matches files named module.html | |||
const MODULE_HTML_EXTENSION_REGEX = new RegExp(/(\.module\/module\.html)/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but I'm pretty sure this would also match if there was something after the .html
extension. For example, my-module.module/module.htmlanythingelse
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one regex nit, but otherwise lgtm!
Description and Context
Expands existing template utils to help determine which templateType a file is and also implements a method to get annotations from source.
Adding these to support HubSpot/hubspot-cms-vscode#117
Screenshots
TODO
Who to Notify