-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adding blueprints for documented ember objects #80
base: master
Are you sure you want to change the base?
Conversation
… and routes. Copies all the same logic in each index.js, but changes the templates
@@ -0,0 +1 @@ | |||
{{yield}} |
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.
I think the template can be removed since it's no different from the original blueprint.
I'd be worried about maintaining this long term. What if, instead, the generator inserted the comments into files? |
@@ -0,0 +1,77 @@ | |||
/* eslint-env node */ | |||
// Note - this is the exact same as ember's normal component index.js |
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.
I haven't dug deep enough into this PR yet so this may be a premature question, but if this content is the same as what is already present why do we need this file?
@@ -0,0 +1,147 @@ | |||
/* eslint-env node */ |
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.
Is this file like blueprints/component/index.js, where its the same content as already exists? If so, the same question I posed for blueprints/component/index.js also applies to this one.
/* eslint-env node */ | ||
// Note - this is the exact same as ember's normal component index.js | ||
|
||
var stringUtil = require('ember-cli-string-utils'); |
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.
Lines 4-8 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
}, | ||
|
||
locals: function(options) { | ||
var templatePath = ''; |
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.
Lines 56-58 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
// if we're in an addon, build import statement | ||
if (options.project.isEmberCLIAddon() || options.inRepoAddon && !options.inDummy) { | ||
if (options.pod) { | ||
templatePath = './template'; |
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.
Does not follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
if (options.pod) { | ||
templatePath = './template'; | ||
} else { | ||
templatePath = pathUtil.getRelativeParentPath(options.entity.name) + |
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.
Does not follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
templatePath = pathUtil.getRelativeParentPath(options.entity.name) + | ||
'templates/components/' + stringUtil.dasherize(options.entity.name); | ||
} | ||
importTemplate = 'import layout from \'' + templatePath + '\';\n'; |
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.
Lines 67-68 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
@@ -0,0 +1,147 @@ | |||
/* eslint-env node */ | |||
|
|||
var fs = require('fs-extra'); |
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.
Lines 3-7 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace
/* eslint-env node */ | ||
// Note - this is the exact same as ember's normal component index.js | ||
|
||
var stringUtil = require('ember-cli-string-utils'); |
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 not use var
anywhere in this file per https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#variables
@@ -0,0 +1,147 @@ | |||
/* eslint-env node */ | |||
|
|||
var fs = require('fs-extra'); |
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 not use var
anywhere in this file per https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#variables
templatePath = pathUtil.getRelativeParentPath(options.entity.name) + | ||
'templates/components/' + stringUtil.dasherize(options.entity.name); | ||
} | ||
importTemplate = 'import layout from \'' + templatePath + '\';\n'; |
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.
re: @GCheung55
That is not how blueprints work nor the recommended way of achieving this (desired) outcome. As far as maintaining long term each release of Ember CLI will require the review of these blueprints for compatibility. Which brings me to how I would actually like to see these improvements approached. Rather than
I would like to see this style guide converted into an Ember CLI addon that provides these blueprints. That way when the blueprints are updated a consumer does not have to remember to copy and paste the files again to their application, but instead merely update the version of this addon and automatically receive the new versions. The onus will still be on this repo's maintainers (such as myself) to keep the blueprints in sync with Ember CLI versions but it would just be part of the process to do so when performing an upgrade of the |
@notmessenger for whatever reason I didn't realize this style guide was not an addon. Turning it into one would be nice. I should have been clearer about my comment on a generator inserting comments into files, my apologies - though I understand my clarification may not change your opinion. What I meant is that the blueprints of "this" addon could trigger the corresponding blueprints, and then insert the comments into the files. It would make maintenance a little easier. E.g. |
Creates blueprints of components, controllers, mixins, and routes with the added documentation specified in ember-style-guide. Copies all the same logic in each index.js, but changes the templates