Skip to content
This repository has been archived by the owner on Jun 10, 2021. It is now read-only.

Extensions glob matching #67

Merged
merged 3 commits into from May 3, 2016
Merged

Extensions glob matching #67

merged 3 commits into from May 3, 2016

Conversation

keyan
Copy link
Contributor

@keyan keyan commented Mar 3, 2016

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 #30 and #56. Also, unrelated but #59 should be closed as it is resolved.

@modocache
Copy link
Contributor

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 added 3 commits May 2, 2016 23:57
Removes logic that makes `--extensions` or `--include-extensionless`
required. Instead will default to searching all extensions. To support
the above and to complete a feature request, also adds glob matching.
Fixes #56 and #30.
@keyan
Copy link
Contributor Author

keyan commented May 3, 2016

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
Copy link
Contributor

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 modocache merged commit a19b2e4 into facebookarchive:master May 3, 2016
@modocache
Copy link
Contributor

Thanks, @keyan!! 💐

@keyan keyan deleted the kp_extensions_glob_matching branch May 3, 2016 04:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Match all extensions / --extensions should accept glob arguments
3 participants