search

facebookarchive / codemod

Codemod is a tool/library to assist you with large-scale codebase refactors that can be partially automated but still require human oversight and occasional intervention. Codemod was developed at Facebook and released as open source.
Apache License 2.0
4.11k stars 198 forks source link

Extensions glob matching #67

Closed keyan closed 8 years ago

keyan commented 8 years ago

This PR adds pattern matching to the --extensions argument. It also changes the script to not require either one of --extensions or --include-extensionless in order to run. Instead if --extensions is not provided it will always default to the Unix pattern *. Includes some code cleanup to support this change.

Open to hearing your thoughts on this. I'm guessing the potentially controversial bit will be defaulting to * and not requiring the above args.

The only strange behavior I noticed with this change is that if you are invoking the program with a wildcard extension you need to add a comma for argparse to work:

codemod --extensions *  '<match>' '<change>'            -->        fails
codemod --extensions *, '<match>' '<change>'            -->        works fine

Not sure how important this is if we keep the default * argument though.

This resolves https://github.com/facebook/codemod/issues/30 and https://github.com/facebook/codemod/issues/56. Also, unrelated but https://github.com/facebook/codemod/issues/59 should be closed as it is resolved.

modocache commented 8 years ago

Thanks!! Sorry for not providing feedback on this sooner, though! 😅

Overall this looks great! Unfortunately you'll have to rebase this in order to get it back into a mergeable state. I think #69 caused a merge conflict.

The only strange behavior I noticed with this change is that if you are invoking the program with a wildcard extension you need to add a comma for argparse to work

Strange indeed!! I suppose since that's now the default, there's no need for users to ever invoke the script with --extensions *, right? Also, could it be that the shell is doing something weird here? Does --extensions "*" behave any differently?

In any case, this looks great. Please rebase and I'll merge (or if you can't be bothered, I'll rebase it myself).

Thanks for pointing out #59 was already fixed--I closed it. 🙇

keyan commented 8 years ago

No problem, thanks for the review. When I opened this PR I did some digging and it looks like this script has a long history! Not sure when during that history you become the de facto maintainer haha.

I just rebased out one of my commits because it did the exact same thing as #69. Also running --extensions "*" works 👍 , but yeah I guess it doesn't really matter as it is the default and it probably won't ever be run with that arg.

modocache commented 8 years ago

Not sure when during that history you become the de facto maintainer haha.

The original maintainer no longer works at Facebook, and without a clear successor this repository was in danger of being archived. Still, several Facebook employees are die-hard users, and the maintenance burden is so low, so I figured: "why not?" 😅

I just rebased out one of my commits because it did the exact same thing as #69.

Oomph, my bad. I didn't realize, should have just cherry-picked that commit. Sorry!

modocache commented 8 years ago

Thanks, @keyan!! 💐