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

Added more detailed param info to actions.js #486

Merged
merged 1 commit into from
Feb 9, 2014

Conversation

AshKyd
Copy link
Contributor

@AshKyd AshKyd commented Jan 31, 2014

Updated the jsdoc in actions.js to make the operation of params on various functions clearer. Let me know if there's anything you think could be clearer.

@@ -357,8 +357,10 @@ actions.directory = function directory(source, destination, process) {
/**
* Copies recursively the files from source directory to root directory.
*
* @param {String} source
* @param {String} destination
* An optimized copy method for larger file trees. See this.bulkCopy.
Copy link
Member

Choose a reason for hiding this comment

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

An "optimized copy method" is not really the words to use. It is not optimized, it strip operations to allow copying large files tree. You should never use this method, unless there's no other solutions. I think this warning should be there (on both bulk methods).

@SBoudrias
Copy link
Member

Can you align description together, e.g.:

 * @param {String} source       Source file to copy from. Relative to this.sourceRoot()
 * @param {String} destination  Destination file to write to. Relative to this.destinationRoot()

@SBoudrias
Copy link
Member

Hey @AshKyd will you be able to take care of it?

@AshKyd
Copy link
Contributor Author

AshKyd commented Feb 7, 2014

Yep, for sure. I'll have another go over the weekend.

@AshKyd
Copy link
Contributor Author

AshKyd commented Feb 9, 2014

Hi @SBoudrias, thanks for bearing with me. I've made these changes.

I've renamed bulkCopy to “a simplified copy method” and added your warning. I'm still not sure that's right though, do you have any preference on wording?

@@ -124,11 +124,12 @@ actions.copy = function copy(source, destination, process) {
* https://github.com/yeoman/generator/pull/359
* https://github.com/yeoman/generator/issues/350
*
* An optimized copy method for larger file trees. Does not do
* A simplified copy method for larger file trees. Does not do
Copy link
Member

Choose a reason for hiding this comment

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

Try something like: "A copy method skiping templating and conflict checking. It will allow copying a large amount of files without causing too much recursion errors. You should never use this method, unless there's no other solutions."

@SBoudrias
Copy link
Member

Hey, see my last comment and squash your commits into a single one. Then we'll be good to merge.

Thanks a lot!

@AshKyd
Copy link
Contributor Author

AshKyd commented Feb 9, 2014

Done.

@SBoudrias
Copy link
Member

Thanks a lot, feel free to continue helping if there's other malformatted sections!

SBoudrias added a commit that referenced this pull request Feb 9, 2014
Added more detailed param info to actions.js
@SBoudrias SBoudrias merged commit e4627d5 into yeoman:master Feb 9, 2014
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.

2 participants