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

Add jsdoc comments for migrate #255

Merged
merged 12 commits into from
Feb 8, 2018

Conversation

dylanonelson
Copy link
Contributor

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
Copy link

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

@evenstensberg
Copy link
Member

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

@dylanonelson
Copy link
Contributor Author

Done!

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@fokusferit
Copy link
Contributor

@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
Copy link
Member

Yup, +1

@dylanonelson
Copy link
Contributor Author

OK sounds good.

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

Choose a reason for hiding this comment

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

returns

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

Object

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

Choose a reason for hiding this comment

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

Node

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Object, Node for ast

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Could you add this JSdoc too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Object and node

* 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
Copy link
Member

Choose a reason for hiding this comment

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

String and Node

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

Choose a reason for hiding this comment

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

Big O in Object and Node

* 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
Copy link
Member

Choose a reason for hiding this comment

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

Node and elaboration in returns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Elaborate what p returns a bit more 👍

Copy link
Contributor Author

@dylanonelson dylanonelson Feb 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

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

Choose a reason for hiding this comment

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

Big O and Node

@evenstensberg
Copy link
Member

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

@dylanonelson
Copy link
Contributor Author

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
Copy link
Member

Re commented, thanks for checking it out!

@evenstensberg
Copy link
Member

Thanks! Will re-review later today! ♥️

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

interactive verb? replaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

its a small pick, so no worries 👍

@@ -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`
Copy link
Member

Choose a reason for hiding this comment

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

interactive indication, so: replaces

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

Choose a reason for hiding this comment

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

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

* @return {Node} []
*
* @param {Node} path - object expression ast
* @returns {Node} path
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what the returns value does/is?

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

Choose a reason for hiding this comment

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

Could you elaborate? Thanks 🎧

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@evenstensberg evenstensberg left a comment

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

its a small pick, so no worries 👍

@evenstensberg
Copy link
Member

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

@webpack-bot
Copy link

@dylanonelson Thanks for your update.

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

@ematipico Please review the new changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants