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.

Sign up for GitHub

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

Jump to bottom

Added support for passing Regex into CLI #14

Merged
merged 5 commits into from
Feb 21, 2017
Merged

Added support for passing Regex into CLI #14

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fbcdc79
Added support for passing Regex into CLI
ryan-codingintrigue Feb 15, 2017
97ddf18
- Fixed style inconsistencies
Feb 16, 2017
2339f3d
Replaced -isRegex with —isRegex and moved to the end of the param list
Feb 17, 2017
e4952af
Merge branch 'feature/add_regex_flag' into develop
Feb 17, 2017
299767e
Added line and sample syntax for CLI in README
Feb 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions bin/cli.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

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 •
edited
Loading

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?

return new RegExp(pattern, flags);
};

//Collect main arguments
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 •
edited
Loading

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.


//Single star globs already get expanded in the command line
const files = argv._.reduce((files, file) => {
Expand Down