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

Add template utils #534

Merged
merged 8 commits into from
Sep 13, 2021
Merged

Add template utils #534

merged 8 commits into from
Sep 13, 2021

Conversation

anthmatic
Copy link
Contributor

@anthmatic anthmatic commented Aug 6, 2021

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

@@ -118,6 +118,49 @@ const PROJECT_TEMPLATE_TYPES = {
},
};

const TEMPLATE_TYPES = {
Copy link
Contributor

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.

@@ -27,6 +31,16 @@ const getFileAnnotations = filePath => {
}
};

const getAnnotationsFromSource = source => {
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

* Returns true if:
* filename is module.html, inside of *.module folder
*/
const isModuleHTMLFile = filePath => MODULE_HTML_EXTENSION_REGEX.test(filePath);
Copy link
Contributor

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?

Copy link
Contributor Author

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');
Copy link
Contributor Author

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);
};

Copy link
Contributor Author

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

Copy link
Contributor

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 🙌

@@ -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)/);
Copy link
Contributor

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

Copy link
Contributor

@brandenrodgers brandenrodgers left a 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!

@anthmatic anthmatic merged commit 3f51251 into master Sep 13, 2021
@anthmatic anthmatic deleted the add/template-utils branch September 13, 2021 14:05
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.

3 participants