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

Reorganized utilities and handled multiline comments in JSON #10

Merged
merged 10 commits into from
Jan 23, 2023

Conversation

ijlee2
Copy link
Owner

@ijlee2 ijlee2 commented Jan 23, 2023

Description

Refactors

I reorganized the utility functions so that their purpose and relation to one another is more clear. In addition, I wrote unit tests that had been missing.

Bug fix

Prior to this pull request, sanitizeJson() didn't remove multiline comments in a JSON file, so JSON.parse() would have resulted in a runtime error. (This case isn't likely if ember-cli-typescript had created tsconfig.json.)

I installed strip-json-comments to remove the inline and multiline comments from tsconfig.json. In the future, I may look at whether there's an easy way to preserve comments and partially update a JSON.

@ijlee2 ijlee2 added bug Something isn't working enhance: documentation Issue asks for better documentation (e.g. README, code, tests) labels Jan 23, 2023
@@ -49,6 +49,7 @@
"@babel/eslint-parser": "^7.19.1",
"glob": "^8.1.0",
"lodash.template": "^4.5.0",
"strip-json-comments": "^5.0.0",
Copy link
Owner Author

Choose a reason for hiding this comment

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

We can remove this dependency if we can find another (simple) way to partially update tsconfig.json.

Comment on lines +23 to +24
"concurrently": "<%= context.projectRoot.devDependencies['concurrently'] %>",
"prettier": "<%= context.projectRoot.devDependencies['prettier'] %>"
Copy link
Owner Author

Choose a reason for hiding this comment

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

Addressed a technical risk (hard-coded latest versions).

Comment on lines +4 to +5
export function processTemplate(file, data) {
const settings = {
Copy link
Owner Author

@ijlee2 ijlee2 Jan 23, 2023

Choose a reason for hiding this comment

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

I renamed the variables so that the words context and options don't carry more than one meaning.

Comment on lines +4 to +5
test('utils | blueprints | process-template > escape', function () {
const blueprintFile = '<%- context.htmlCode %>';
Copy link
Owner Author

Choose a reason for hiding this comment

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

At the moment, none of the blueprints make use of the escape feature from lodash.template.

@ijlee2 ijlee2 changed the title Reorganized utilities Reorganized utilities and handled multiline comments in JSON Jan 23, 2023
@ijlee2 ijlee2 marked this pull request as ready for review January 23, 2023 10:55
@ijlee2 ijlee2 merged commit d85a39c into main Jan 23, 2023
@ijlee2 ijlee2 deleted the refactor-code-part-7 branch January 23, 2023 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhance: documentation Issue asks for better documentation (e.g. README, code, tests)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant