Add jsdoc comments for migrate #255

dylanonelson · 2018-01-28T18:52:36Z

What kind of change does this PR introduce?
JSdocs for migrate

Did you add tests for your changes?
No

If relevant, did you update the documentation?
n/a

Summary
Resolves issue #219

Does this PR introduce a breaking change?
No

Other information
Did my best to describe everything correctly, but I may have made mistakes. I noticed the existing jsdoc comments were a bit inconsistent, so I went with what made sense to me.

webpack-bot · 2018-01-28T18:59:28Z

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

evenstensberg · 2018-01-28T22:39:43Z

Nice, thanks! Can you run the jsdocs command and commit?

dylanonelson · 2018-01-29T00:47:35Z

Done!

evenstensberg

LGTM

fokusferit · 2018-01-30T09:56:55Z

@dylanonelson thanks for the work! As this PR is quite huge (due to generation of docs), I'd like to invest at least some time to go through the comments :)

And thanks to your PR, I think for the future doc generation should be part of CI not of each PR.

evenstensberg · 2018-01-30T17:48:12Z

Yup, +1

dylanonelson · 2018-01-30T18:21:55Z

OK sounds good.

ematipico · 2018-01-29T14:17:14Z

lib/migrate/resolve/resolve.js

+	 * based on what is already in `resolve.root`
+	 *
+	 * @param {NodePath} p — wrapped ast property node that represents the `resolve` property
+	 * @return {NodePath} p


evenstensberg

Okay had an extensive review, most of the stuff are okay, except some small typos regarding to the syntax we have. Collection is identical to Node, but we use Node - ast - returns JScodeshift ast for that. Thank you so much for the PR, its huge, extensive, important and well done!

evenstensberg · 2018-01-30T21:41:55Z

lib/migrate/bannerPlugin/bannerPlugin.js

+ * Transform for BannerPlugin arguments. Consolidates first and second argument (if
+ * both are present) into single options object.
+ *
+ * @param {object} j — jscodeshift top-level import


evenstensberg · 2018-01-30T21:42:29Z

lib/migrate/bannerPlugin/bannerPlugin.js

+ *
+ * @param {object} j — jscodeshift top-level import
+ * @param {Collection} ast — jscodeshift Collection to transform
+ * @returns {Collection} ast — jscodeshift Collection


evenstensberg · 2018-01-30T21:43:04Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+ * @param {object} j — jscodeshift top-level import
+ * @param {Node} node — ast node to check
+ * @param {string} pluginName
+ * @returns {boolean}


Boolean, Object, String, could you add pluginName - name of the plugin?

evenstensberg · 2018-01-30T21:43:29Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+ *
+ * Transform for ExtractTextPlugin arguments. Consolidates arguments into single options object.
+ *
+ * @param {object} j — jscodeshift top-level import


Object, Node for ast

evenstensberg · 2018-01-30T21:43:48Z

lib/migrate/index.js

@@ -48,7 +48,7 @@ function transformSingleAST(ast, source, transformFunction) {
 * @param { String } source - Source file contents
 * @param { Array<Function> } transformations - List of trnasformation functions in defined the
 * order to apply. By default all defined transfomations.
- * @param { Object } options - Reacst formatting options
+ * @param { Object } options - Recast formatting options


Could you add this JSdoc too?

Sure, no problem. Do you mean the jsdoc for the properties of the options object?

No, you forgot to do this/add the extra *. And Array<Function> is wrong, should be Function[]

evenstensberg · 2018-01-30T21:49:33Z

lib/migrate/removeJsonLoader/removeJsonLoader.js

+ * Transform which removes the json loader from all possible declarations
+ *
+ * @param {object} j — jscodeshift top-level import
+ * @param {Collection} ast — jscodeshift Collection to transform


Object and node

evenstensberg · 2018-01-30T21:49:42Z

lib/migrate/removeJsonLoader/removeJsonLoader.js

+	 * Remove the loader with name `name` from the given NodePath
+	 *
+	 * @param {NodePath} path — NodePath to remove the loader from
+	 * @param {string} name — the name of the loader to remove


String and Node

evenstensberg · 2018-01-30T21:49:59Z

lib/migrate/resolve/resolve.js

+ *
+ * Transform which consolidates the `resolve.root` configuration option into the `resolve.modules` array
+ *
+ * @param {object} j — jscodeshift top-level import


Big O in Object and Node

evenstensberg · 2018-01-30T21:50:18Z

lib/migrate/resolve/resolve.js

+	 * Add a `modules` property to the `resolve` object or update the existing one
+	 * based on what is already in `resolve.root`
+	 *
+	 * @param {NodePath} p — wrapped ast property node that represents the `resolve` property


Node and elaboration in returns

Not sure what you mean here by “elaboration in returns”?

Elaborate what p returns a bit more 👍

I added a description to this @returns tag. Do you want me to add descriptions for all of them? In cases where it seemed self-explanatory I omitted the description.

evenstensberg · 2018-01-30T21:50:37Z

lib/migrate/uglifyJsPlugin/uglifyJsPlugin.js

+ *
+ * Transform which adds a `sourceMap: true` option to instantiations of the UglifyJsPlugin
+ *
+ * @param {object} j — jscodeshift top-level import


Big O and Node

evenstensberg · 2018-01-30T21:52:26Z

Planned to have this merged and fixing that myself, but let's just fix it now 🍶

dylanonelson · 2018-01-31T23:50:03Z

OK I did my best to make all the requested changes and also scanned through all the docs to make sure they matched your description—{Node} instead of {Collection} and jscodeshift ast instead of jscodeshift Collection, etc. I left a couple of questions on your comments, the big one being what you meant in lib/migrate/index.js about adding another jsdoc on L51.

Thanks for the review!

evenstensberg · 2018-02-01T00:28:57Z

Re commented, thanks for checking it out!

evenstensberg · 2018-02-02T06:42:33Z

Thanks! Will re-review later today! ♥️

evenstensberg

Thanks, sorry for getting back so late! So there's some small nits left, sorry for being so strict!

evenstensberg · 2018-02-04T22:17:00Z

lib/migrate/loaders/loaders.js

@@ -219,7 +248,7 @@ module.exports = function(j, ast) {
 			.forEach(createArrayExpressionFromArray);

 	/**
-	 * Finds loaders with options encoded as query string and replaces it with options obejct
+	 * Find loaders with options encoded as a query string and replace the string with an options object


interactive verb? replaces

I'm a little confused about this. You want to change replace to replaces but leave Find in the same sentence? It seems like they should match. I skimmed through some of the other comments and it looks like sometimes the doclets use the replace version and sometimes they use the replaces version. If you want, I can make them all consistent. Or there may be something special about this one case that I don't understand—just let me know.

its a small pick, so no worries 👍

evenstensberg · 2018-02-04T22:17:20Z

lib/migrate/loaders/loaders.js

@@ -249,7 +277,7 @@ module.exports = function(j, ast) {
 			.replaceWith(createLoaderWithQuery);

 	/**
-	 * Finds nodes with `query` key and replaces it with `options`
+	 * Find nodes with a `query` key and replace it with `options`


interactive indication, so: replaces

evenstensberg · 2018-02-04T22:18:31Z

lib/migrate/extractTextPlugin/extractTextPlugin.js

+ * @param {Object} j - jscodeshift top-level import
+ * @param {Node} node - ast node to check
+ * @param {String} pluginName - name of the plugin
+ * @returns {Boolean}


Could you give a bit of description what the boolean indicates in the @returns ?

evenstensberg · 2018-02-04T22:19:10Z

lib/migrate/loaders/loaders.js

-	 * @return {Node}      []
+	 *
+	 * @param  {Node} path - object expression ast
+	 * @returns {Node} path


Could you explain what the returns value does/is?

evenstensberg · 2018-02-04T22:19:21Z

lib/migrate/loaders/loaders.js

-	 * @param  {Node} path [description]
-	 * @return {Node}      [description]
+	 * @param {Node} path - identifier ast
+	 * @returns {Boolean}


Could you elaborate? Thanks 🎧

evenstensberg · 2018-02-04T22:19:48Z

lib/migrate/loaders/loaders.js

-	 * @param {Node} p []
-	 * @returns {*}		[]
+	 * @param {Node} p - object expression ast that has a key for either 'preLoaders' or 'postLoaders'
+	 * @returns {Node} p


Would be a 10/10 if there was a small explaining part here as well

evenstensberg

LGTM, thank you for bearing with me on this! 💙

evenstensberg · 2018-02-05T23:24:53Z

lib/migrate/loaders/loaders.js

@@ -219,7 +248,7 @@ module.exports = function(j, ast) {
 			.forEach(createArrayExpressionFromArray);

 	/**
-	 * Finds loaders with options encoded as query string and replaces it with options obejct
+	 * Find loaders with options encoded as a query string and replace the string with an options object


its a small pick, so no worries 👍

evenstensberg · 2018-02-05T23:26:27Z

@ematipico and @fokusferit can you re-review? Thanks bois!

webpack-bot · 2018-02-07T16:36:51Z

@dylanonelson Thanks for your update.

I labeled the Pull Request so reviewers will review it again.

@ematipico Please review the new changes.

dylanonelson added 3 commits January 27, 2018 23:33

docs: jsdoc comments for migrate/loaders

11df466

docs: jsdoc comments for migrate transforms

c0ed4fb

docs: fix typos & omissions

8dbf62f

webpack-bot added the PR: unreviewed label Jan 28, 2018

webpack-bot added the PR: CI-ok label Jan 28, 2018

docs: run jsdoc command

8065290

remove jsdoc exclude in config and rerun jsdocs

bb5ba98

evenstensberg approved these changes Jan 29, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: unreviewed labels Jan 29, 2018

evenstensberg requested review from fokusferit and ematipico January 29, 2018 20:55

ematipico suggested changes Jan 30, 2018

View reviewed changes

webpack-bot added the PR: reviewed-changes-requested label Jan 30, 2018

evenstensberg requested changes Jan 30, 2018

View reviewed changes

docs(migrate): incorporate pr feedback

041cb05

webpack-bot added PR: review-outdated and removed PR: reviewed-approved PR: reviewed-changes-requested labels Jan 31, 2018

docs: run jsdoc commmand

c0164ac

docs(migrate): incorporate pr feedback

22466e9

dylanonelson added 2 commits January 31, 2018 21:23

docs(migrate): replace em dashes with hyphens

3b3a539

docs: run jsdoc command

d6eb27c

evenstensberg requested changes Feb 4, 2018

View reviewed changes

webpack-bot added PR: reviewed-changes-requested and removed PR: review-outdated labels Feb 4, 2018

docs(migrate): add descriptions to @returns tags

b21871e

webpack-bot added PR: review-outdated and removed PR: reviewed-changes-requested labels Feb 5, 2018

evenstensberg approved these changes Feb 5, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 5, 2018

ematipico approved these changes Feb 7, 2018

View reviewed changes

docs(migrate): run jsdoc command

193e4c1

webpack-bot added PR: review-outdated and removed PR: reviewed-approved labels Feb 7, 2018

evenstensberg approved these changes Feb 8, 2018

View reviewed changes

webpack-bot added PR: reviewed-approved and removed PR: review-outdated labels Feb 8, 2018

evenstensberg merged commit 73fdf7d into webpack:master Feb 8, 2018

evenstensberg mentioned this pull request Feb 14, 2018

[Migrate]: JSdocs for migrate #219

Closed

Add jsdoc comments for migrate #255

Add jsdoc comments for migrate #255

Conversation

dylanonelson commented Jan 28, 2018

webpack-bot commented Jan 28, 2018

evenstensberg commented Jan 28, 2018

dylanonelson commented Jan 29, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

fokusferit commented Jan 30, 2018

evenstensberg commented Jan 30, 2018

dylanonelson commented Jan 30, 2018

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanonelson Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Jan 30, 2018

dylanonelson commented Jan 31, 2018

evenstensberg commented Feb 1, 2018

evenstensberg commented Feb 2, 2018

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evenstensberg commented Feb 5, 2018

webpack-bot commented Feb 7, 2018

dylanonelson Feb 1, 2018 •

edited

Loading