Adding blueprints for documented ember objects by jsfuchs · Pull Request #80 · softlayer/ember-style-guide

jsfuchs · 2017-08-04T14:44:26Z

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

… and routes. Copies all the same logic in each index.js, but changes the templates

GCheung55 · 2017-08-07T17:50:30Z

blueprints/component/files/__root__/__templatepath__/__templatename__.hbs

@@ -0,0 +1 @@
+{{yield}}


I think the template can be removed since it's no different from the original blueprint.

GCheung55 · 2017-08-07T17:58:50Z

I'd be worried about maintaining this long term.

What if, instead, the generator inserted the comments into files?

notmessenger · 2017-08-11T02:30:08Z

blueprints/component/index.js

@@ -0,0 +1,77 @@
+/* eslint-env node */
+// Note - this is the exact same as ember's normal component index.js


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?

notmessenger · 2017-08-11T02:31:36Z

blueprints/route/index.js

@@ -0,0 +1,147 @@
+/* eslint-env node */


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.

notmessenger · 2017-08-11T02:33:44Z

blueprints/component/index.js

+/* eslint-env node */
+// Note - this is the exact same as ember's normal component index.js
+
+var stringUtil         = require('ember-cli-string-utils');


Lines 4-8 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:33:56Z

blueprints/component/index.js

+  },
+
+  locals: function(options) {
+    var templatePath   = '';


Lines 56-58 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:34:12Z

blueprints/component/index.js

+    // if we're in an addon, build import statement
+    if (options.project.isEmberCLIAddon() || options.inRepoAddon && !options.inDummy) {
+      if (options.pod) {
+        templatePath   = './template';


Does not follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:34:25Z

blueprints/component/index.js

+      if (options.pod) {
+        templatePath   = './template';
+      } else {
+        templatePath   = pathUtil.getRelativeParentPath(options.entity.name) +


Does not follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:34:34Z

blueprints/component/index.js

+        templatePath   = pathUtil.getRelativeParentPath(options.entity.name) +
+                         'templates/components/' + stringUtil.dasherize(options.entity.name);
+      }
+      importTemplate   = 'import layout from \'' + templatePath + '\';\n';


Lines 67-68 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:34:53Z

blueprints/route/index.js

@@ -0,0 +1,147 @@
+/* eslint-env node */
+
+var fs                   = require('fs-extra');


Lines 3-7 don't follow this own style guide ;-) https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#whitespace

notmessenger · 2017-08-11T02:36:06Z

blueprints/component/index.js

+/* eslint-env node */
+// Note - this is the exact same as ember's normal component index.js
+
+var stringUtil         = require('ember-cli-string-utils');


Should not use var anywhere in this file per https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#variables

notmessenger · 2017-08-11T02:36:30Z

blueprints/route/index.js

@@ -0,0 +1,147 @@
+/* eslint-env node */
+
+var fs                   = require('fs-extra');


Should not use var anywhere in this file per https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#variables

notmessenger · 2017-08-11T02:41:26Z

blueprints/component/index.js

+        templatePath   = pathUtil.getRelativeParentPath(options.entity.name) +
+                         'templates/components/' + stringUtil.dasherize(options.entity.name);
+      }
+      importTemplate   = 'import layout from \'' + templatePath + '\';\n';


Template literals (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) should be used per https://github.com/softlayer/ember-style-guide/blob/master/javascript.md#es2015-and-es2016

notmessenger · 2017-08-11T03:12:32Z

re: @GCheung55

I'd be worried about maintaining this long term.
What if, instead, the generator inserted the comments into files?

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

Just copy the blueprints folder to the top level of your Ember app, and it will override the default generators with their documented versions.

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 ember-cli dependency.

GCheung55 · 2017-08-22T18:39:29Z

@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. ember generate jsdoc-component foo-bar would trigger the component blueprint to create foo-bar component, and then the jsdoc-component would insert the comments, maybe as part of the "content".

Jared Fuchs added 3 commits August 4, 2017 10:30

Adding blueprints for documented versions of components, controllers,…

c63437a

… and routes. Copies all the same logic in each index.js, but changes the templates

Update README to mention blueprints folder and how to use.

dc98cf4

Adding Mixin blueprint as well.

7cd9013

GCheung55 reviewed Aug 7, 2017

View reviewed changes

notmessenger reviewed Aug 11, 2017

View reviewed changes

		@@ -0,0 +1,77 @@
		/* eslint-env node */
		// Note - this is the exact same as ember's normal component index.js

		@@ -0,0 +1,147 @@
		/* eslint-env node */

		var fs = require('fs-extra');

		@@ -0,0 +1 @@
		{{yield}}

Conversation

jsfuchs commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GCheung55 commented Aug 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmessenger commented Aug 11, 2017

Uh oh!

GCheung55 commented Aug 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants