Skip to content
This repository was archived by the owner on Mar 26, 2018. It is now read-only.

Added options.copy #21

Closed
wants to merge 5 commits into from
Closed

Added options.copy #21

wants to merge 5 commits into from

Conversation

ProLoser
Copy link

@ProLoser ProLoser commented Feb 5, 2014

If undefined (null) then the old behavior takes precedence regarding destination deciding if the files are copied or moved.

However, you can now override this behavior in either direction.

options.copy = true will always copy, regardless of if destination is defined or not
options.copy = false will always move, regardless of if destination is defined or not

@jamesplease
Copy link

It seems likely to me that there's a way to solve #20 without introducing a new option.

@ProLoser
Copy link
Author

@jmeas perhaps just for fixing #20, however this PR addresses far more use-cases than just that outlined in issue #20 and I still believe it is a more versatile solution.

I could have simply added the behavior I was describing, but then someone might have requested the ability to move even though a destination is specified. I do not believe that moving should be mutually exclusive to specifying destinations.

@eddiemonge
Copy link
Member

needs tests

@mwillerich
Copy link

@eddiemonge @ProLoser I've written some tests and doc, any feedback? Is this the right way of contributing to this, btw?

tests and documentation for copy option
@ProLoser
Copy link
Author

@mwillerich that works, thanks! I'm no longer working at the company that used these changes so getting tests became less relevant to my urgent needs lol. In the future, if I (the middle-man) appear to be no longer active you can always fork off of my work and open a new pull request directly into the yeoman fork and just bypass my own.

@eddiemonge I merged @mwillerich's tests

dirname = el.dest ? el.dest : path.dirname(file);
resultPath = path.resolve(dirname, newName);

if (options.copy === null) {
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be if (!options.copy) {

Copy link
Author

Choose a reason for hiding this comment

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

It's a 3-state option. True, False and null. Perhaps the check should be rearranged so that it explicitly does options.copy === true, options.copy === false and then just use this case for the rest. Since null != undefined

@ProLoser
Copy link
Author

I'm honestly no longer working or using this code whatsoever so you guys are better off using @mwillerich's fork.

@mwillerich
Copy link

I have opened new PR #31 from my fork.

@eddiemonge eddiemonge closed this May 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants