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

Passes full file path to the replacement function #67

Closed
wants to merge 3 commits into from

Conversation

oleggromov
Copy link
Contributor

Larry, hi!

The PR solves #50 issue passing file name with a full path in all possible cases:

  • the replace is done on a stream, with a regex or string passed like a replacement
  • the replace is applied to a buffer, also no matter if one wants to match a string or a regex

New option

In order to use this functionality one should use new option passFilename which should be set to true (as well as pass a function as a replacement), and therefore the callback will be called with two arguments, where the second is file path.

Callback signature

In the previous version callback could have been invoked with only 1 argument or with a bunch of them as standard String.replace do.

Backwards compatibility

As mentioned above, the new option is proposed to keep users in safe backwards compatibility state after an update to the last version.

ToDo and questions

The PR is not completed yet because of the absence of tests and documentation. I'll add them as soon as #66 would have been merged. Please look at both PRs and let me know if something is wrong or you had another evolution plan.

The main question is, should the major release be introduced only because of backwards incompatibility, or should it be done just because of the existing evolution plan?

@oleggromov
Copy link
Contributor Author

Hey Larry, where're you?!

if (options.passFilename && typeof replacement === 'function') {
var userReplacement = replacement;
replacement = function (match) {
userReplacement(match, file.path);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Nitpick: Semicolon please!

@lazd
Copy link
Owner

lazd commented Mar 30, 2016

@oleggromov this won't work, we need all of the arguments that are passed to the function when doing a regex replace with function :-\

if (options.passFilename && typeof replacement === 'function') {
var userReplacement = replacement;
replacement = function (match) {
userReplacement(match, file.path);
Copy link
Owner

Choose a reason for hiding this comment

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

Another potential solution would be to call the replacement function in the context of the file object:

userReplacement.apply(file, arguments);
// this.path == the path of the file

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea! But maybe it's better and safer to create context object and set its properties explicitly? It will help us with argument immutability.

var context = {
  filepath: file.path
};
userReplacement.apply(context, arguments);

@oleggromov oleggromov mentioned this pull request Apr 4, 2016
@lazd
Copy link
Owner

lazd commented Jun 20, 2017

I went ahead and used the approach in #80 for this. Thanks for your contribution; though it wasn't merged, it still helped define the API and the project is better for it.

@lazd lazd closed this Jun 20, 2017
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