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

Return path in rename by function example #57

Merged
merged 1 commit into from
Sep 25, 2015

Conversation

florianeckerstorfer
Copy link
Contributor

At least the only way I got renaming by function to work is by adding return path; to the function.

At least the only way I got renaming by function to work is by adding `return path;` to the function.
yocontra pushed a commit that referenced this pull request Sep 25, 2015
Return path in rename by function example
@yocontra yocontra merged commit ba5ab6d into hparra:master Sep 25, 2015
@shinnn
Copy link
Collaborator

shinnn commented Sep 25, 2015

Sorry for the late response but I'd rather revert this. Whether the path object is returned or not doesn't affect renaming result. This documentaton change would just confuses users.

At least the only way I got renaming by function to work is by adding return path; to the function.

@florianeckerstorfer Can you provide an example that can reproduce the problem?

@yocontra
Copy link
Collaborator

@shinnn I only merged this because I also had this problem before, so I guess we should open a ticket for fixing this then revert the doc change.

@yocontra
Copy link
Collaborator

I think the return is a better API decision though IMO - otherwise people will try to do async shit in there and it won't work

@shinnn
Copy link
Collaborator

shinnn commented Sep 25, 2015

I think the return is a better API decision though IMO

Previously gulp-rename checked the return value first, and used the original path object as a fallback, but it was removed in #24.

@florianeckerstorfer
Copy link
Contributor Author

After checking again I could not reproduce the error, maybe I was on a earlier version, I don't know. I kinda expected the return, because when I was reading the docs I was wondering "no return?" and thats why I created the PR prematurely. Sorry for that.

@shinnn
Copy link
Collaborator

shinnn commented Dec 9, 2016

Reverted in #72.

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.

3 participants