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 support for passing Regex into CLI #14

Merged
merged 5 commits into from
Feb 21, 2017

Conversation

ryan-codingintrigue
Copy link
Contributor

@ryan-codingintrigue ryan-codingintrigue commented Feb 15, 2017

This will allow for passing the from/to parameters as regex in the format /match/g through the CLI. If pattern does not match a Regex it will fall back to passing through as a String.

This will allow for passing the `from`/`to` parameters as regex in the format `\match\g` through the CLI. If pattern does not match a Regex it will fall back to passing through as a `String`.
@coveralls
Copy link

coveralls commented Feb 15, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling fbcdc79 on ryan-codingintrigue:master into f176f67 on adamreisnz:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fbcdc79 on ryan-codingintrigue:master into f176f67 on adamreisnz:master.

Copy link
Owner

@adamreisnz adamreisnz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, could you address the comments and update the readme before I merge it in?

bin/cli.js Outdated
const from = argv._.shift();
const to = argv._.shift();
const from = getRegexOrString(argv._.shift());
const to = getRegexOrString(argv._.shift());
Copy link
Owner

@adamreisnz adamreisnz Feb 15, 2017

Choose a reason for hiding this comment

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

The to property can never be a Regex, so it should always be a regular string.

bin/cli.js Outdated
const getRegexOrString = value => {
if(!isRegexMatch.test(value)) return value;
const flags = value.replace(/.*\/([gimy]*)$/, '$1');
const pattern = value.replace(new RegExp('^/(.*?)/'+flags+'$'), '$1');
Copy link
Owner

@adamreisnz adamreisnz Feb 15, 2017

Choose a reason for hiding this comment

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

This code block will not pass the linter, could you run ESLint on the code to fix style inconsistencies please?

bin/cli.js Outdated
@@ -14,9 +14,18 @@ if (argv._.length < 3) {
process.exit(1);
}

// Parse from/to as either a Regex or a string
const isRegexMatch = /.*\/([gimy]*)$/;
Copy link
Owner

Choose a reason for hiding this comment

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

Slightly worried that this pattern might incorrectly identify for example paths as a regex. E.g. /some-path/g would match, but it could be a path string rather than a regex. Perhaps it'd be better to include a flag if you are passing in a regular expression as the from parameter, to be explicit and prevent accidental regex matches.

I'd propose the flag isRegex for this purpose.

Copy link
Contributor Author

@ryan-codingintrigue ryan-codingintrigue Feb 16, 2017

Choose a reason for hiding this comment

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

This is a good point, thanks for pointing this out. So should we use:

replace-in-file [-regex] from to some/file.js,some/**/glob.js

replace-in-file from [-isRegex] to some/file.js,some/**/glob.js

I'd hoped I could add array support onto all the parameters, but might be a bit too complicated for CLI options.

Copy link
Owner

Choose a reason for hiding this comment

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

I would say:

replace-in-file from to some/file.js,some/**/glob.js --isRegex

Where from then is the regular expression string.

Yes, agreed, the CLI and arrays is a bit hard. I figured at least to use the comma separated list for the files would be most useful.

Ryan Graham added 3 commits February 16, 2017 06:43
- Added -isRegex parameter to explicitly state ‘from’ parameter is a regex
- Removed regex support from ‘to’ parameter
@coveralls
Copy link

coveralls commented Feb 17, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling e4952af on ryan-codingintrigue:master into f176f67 on adamreisnz:master.

Copy link
Contributor Author

@ryan-codingintrigue ryan-codingintrigue left a comment

Choose a reason for hiding this comment

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

Fixed style inconsistencies here and added support for an "--isRegex" flag. When passed, this flag will assume the 'from' parameter is valid regex in the format /abcdxyz/[gimy] otherwise it will throw an error.

@adamreisnz
Copy link
Owner

Thanks that looks alright. Could you update the readme.md as well to include instructions for this parameter?

@coveralls
Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 299767e on ryan-codingintrigue:master into f176f67 on adamreisnz:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 299767e on ryan-codingintrigue:master into f176f67 on adamreisnz:master.

@adamreisnz
Copy link
Owner

Great, thanks!

@adamreisnz adamreisnz merged commit fbc7de4 into adamreisnz:master Feb 21, 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.

None yet

3 participants