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(cli): switch .template to .ejs #996

Merged
merged 2 commits into from
Feb 15, 2018
Merged

feat(cli): switch .template to .ejs #996

merged 2 commits into from
Feb 15, 2018

Conversation

virkt25
Copy link
Contributor

@virkt25 virkt25 commented Feb 15, 2018

Switch .template to .ejs for CLI template files. Gives us syntax highlighting instead of error in VSCode.

Follow up from #991

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • Related API Documentation was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in packages/example-* were updated

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

I am not able to judge the impact of the changes project-generator.js, but I don't see any obvious problem.

FWIW, LGTM.

Please get somebody more familiar with our CLI package to approve your changes too.

@@ -1,4 +1,3 @@
dist
dist6
Copy link
Member

Choose a reason for hiding this comment

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

I am not a fan of making unrelated changes. Can you at least split the removal of dist6 entries into a standalone commit please?

@@ -62,5 +62,3 @@ api-docs/

# Transpiled JavaScript files from Typescript
dist/
dist6/
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

virkt25 added a commit that referenced this pull request Feb 15, 2018
Follow up from #996 and related to dropping Node 6 support. CLI template had references to `dist6` folders which are no longer generated.
@virkt25
Copy link
Contributor Author

virkt25 commented Feb 15, 2018

@bajtos Feedback applied. See #998 for template changes unrelated to name.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@virkt25 there is no test need to upgrade? like compare file name. I am not quite familiar with the cli package, may not be a valid concern, feel free to ignore if tests are good :-p

Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

Same concern as Janny, otherwise LGTM.

@virkt25
Copy link
Contributor Author

virkt25 commented Feb 15, 2018

Current tests assert that most of the files that have been renamed exist in a generated project (with actual name not with .ejs and check contents (usually a line not entire file contents). So I believe we are covered. You can find those tests in packages/cli/test/project.js, app.js, and extension.js in same folder as first test file.

@virkt25 virkt25 merged commit a27e856 into master Feb 15, 2018
@virkt25 virkt25 deleted the ejs branch February 15, 2018 16:32
virkt25 added a commit that referenced this pull request Feb 15, 2018
Follow up from #996 and related to dropping Node 6 support. CLI template had references to `dist6` folders which are no longer generated.
virkt25 added a commit that referenced this pull request Feb 15, 2018
Follow up from #996 and related to dropping Node 6 support. CLI template had references to `dist6` folders which are no longer generated.
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.

5 participants