Skip to content

Use globby instead of a custom directory traversal #772

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

Closed
wants to merge 1 commit into from

Conversation

NickHeiner
Copy link

@NickHeiner NickHeiner commented Feb 24, 2021

Fixes #770.

There are two failing tests around prettier. I looked at them for a bit and couldn't figure it out. Are you able to take a look and see if anything jumps out?

I used --no-verify to bypass the commit formatting rules. If you don't want to squash merge, then I'll reformat my commit to be right once the PR is approved.

I will add docs / update the version number / etc once you approve the basic shape of this.

@@ -5,5 +5,6 @@
"**/*.js": { "when": "$(basename).ts" },
"**/*.d.ts": { "when": "$(basename).ts" },
"**/*.js.map": true
}
},
"typescript.tsdk": "node_modules/typescript/lib"
Copy link
Author

@NickHeiner NickHeiner Feb 24, 2021

Choose a reason for hiding this comment

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

This defaults VSCode to using the node_modules version of TS instead of the version bundled with VSCode itself.

// ignore paths starting with a dot
basename.startsWith('.') ||
// ignore TypeScript declaration files
(basename.endsWith('.d.ts') && type === EntryType.File) ||
Copy link
Author

@NickHeiner NickHeiner Feb 24, 2021

Choose a reason for hiding this comment

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

I'm happy to add this hardcoding back in, but I actually don't know if this exclusion makes sense. Who says .d.ts files never need to be codemodded? 😄 (Same idea applies to automatically ignoring . files.)

// ignore TypeScript declaration files
(basename.endsWith('.d.ts') && type === EntryType.File) ||
// ignore node_modules directories
(basename === 'node_modules' && type === EntryType.Directory)
Copy link
Author

Choose a reason for hiding this comment

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

This is handled automatically by globby's gitignore feature.

@@ -177,11 +177,7 @@ export default class Options {
if (arg[0] === '-') {
throw new Error(`unexpected option: ${arg}`)
} else {
if (hasGlob(arg)) {
config.addSourcePaths(...globSync(arg))
Copy link
Author

Choose a reason for hiding this comment

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

Globbing is now centralized in iterateSources, so no need to do it here as well.

}
): Array<Source> {
return sync(paths, {
gitignore: true,
Copy link
Author

Choose a reason for hiding this comment

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

I didn't add a test for this, because it would be a bit more manual overhead to set up the test files correctly. But I can do it if you feel strongly.

@@ -1,6 +1,6 @@
import { deepEqual, ok, strictEqual } from 'assert'
import * as fs from 'fs'
import { dirname, join } from 'path'
import { dirname, join, resolve as pathResolve } from 'path'
Copy link
Author

Choose a reason for hiding this comment

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

I named it pathResolve because resolve felt so generic that it wouldn't be obvious to other devs that it comes from path.

@lgtm-com
Copy link

lgtm-com bot commented Feb 24, 2021

This pull request introduces 1 alert when merging 3111b56 into 0631098 - view on LGTM.com

new alerts:

  • 1 for Unused variable, import, function or class

@@ -53,16 +53,6 @@ describe('Options', function () {
deepEqual(config.sourcePaths, ['src/', 'a.js'])
})

it('treats sources as globs', function () {
Copy link
Author

Choose a reason for hiding this comment

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

I moved this test to CLITest.ts. That's a more end-to-end test, and is thus more robust in the face of changing internals.

@Nantris
Copy link

Nantris commented Jun 19, 2021

This seems like a great addition. Any update on integrating this?

@eventualbuddha
Copy link
Collaborator

Merged in 4a396f4. Thanks, and sorry for the extremely long delay 😞

eventualbuddha added a commit that referenced this pull request Nov 14, 2021
After #772 made `codemod` respect `.gitignore` files by using the `gitignore` option in `globby`, I pretty quickly ran into sindresorhus/globby#86. There didn't seem to be an obvious workaround for this, so I ended up building the functionality to search up for `.gitignore` files so that e.g. running `codemod` in a subdirectory would respect a `.gitignore` at the root of the git repo.

This commit also contains some more cleanup and fixes which I didn't bother to separate.
eventualbuddha added a commit that referenced this pull request Nov 14, 2021
After #772 made `codemod` respect `.gitignore` files by using the `gitignore` option in `globby`, I pretty quickly ran into sindresorhus/globby#86. There didn't seem to be an obvious workaround for this, so I ended up building the functionality to search up for `.gitignore` files so that e.g. running `codemod` in a subdirectory would respect a `.gitignore` at the root of the git repo.

This commit also contains some more cleanup and fixes which I didn't bother to separate.
eventualbuddha added a commit that referenced this pull request Nov 14, 2021
After #772 made `codemod` respect `.gitignore` files by using the `gitignore` option in `globby`, I pretty quickly ran into sindresorhus/globby#86. There didn't seem to be an obvious workaround for this, so I ended up building the functionality to search up for `.gitignore` files so that e.g. running `codemod` in a subdirectory would respect a `.gitignore` at the root of the git repo.

This commit also contains some more cleanup and fixes which I didn't bother to separate.
@Nantris
Copy link

Nantris commented Jan 12, 2022

Is there any example of how to use Globby globs with Codemod?

@eventualbuddha
Copy link
Collaborator

Is there any example of how to use Globby globs with Codemod?

You should be able to use it with @codemod/cli like normal, but you may need to quote it for your shell to not try to do the expansion:

codemod -p my-plugin 'src/**/*.ts'

@Nantris
Copy link

Nantris commented Jun 8, 2022

Globs that used to work for us seem to affect no files after upgrading. Any ideas? They're pretty straightforward, like ./packages/shared/*/src/**/*.js. I've tried with and without the leading ./

Is this something that some codemods might not play nicely with? I can't think what else the issue might be. Specifically I'm having trouble with import-move-codemod

It still works with explicit paths to individual files, but no longer seems to with globs.

@NickHeiner
Copy link
Author

@slapbox You could also try https://github.com/NickHeiner/jscodemod instead. :)

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.

Suggestion: replace custom file path resolver with globby
3 participants