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

[discussion] Do not use glob for prettier by default #424

Closed
axetroy opened this issue May 21, 2019 · 7 comments · Fixed by #438
Closed

[discussion] Do not use glob for prettier by default #424

axetroy opened this issue May 21, 2019 · 7 comments · Fixed by #438

Comments

@axetroy
Copy link
Contributor

axetroy commented May 21, 2019

Use glob if there should be a wildcard, otherwise, use a normal file path

Otherwise, it is difficult for Prettier to format a specific file.

eg. in deno_std directory.

We want to format the test.ts file.

only this file, no one else.

deno run prettier/main.ts test.ts

Does it look right?

No.

It will format all files under deno_std directory which match with /test\.ts/g.

It is difficult to specify a file to format.

Solution

This is the solution I think:

  1. Use glob when wildcards appear, Otherwise use path.

  2. Don't walk the entire directory

deno_std/prettier/main.ts

Lines 245 to 253 in 47134db

args.length > 0
? args.map((a: string): RegExp => glob(a, options))
: undefined;
const files = walk(".", { match, skip });
try {
if (check) {
await checkSourceFiles(files, prettierOpts);
} else {
await formatSourceFiles(files, prettierOpts);

BREAKING CHANGES

  1. without wildcards, It will select the specified file instead of a matching file.

Before

> deno run prettier/main.ts test.ts

Formatting test.ts
Formatting fs/test.ts
Formatting fs/copy_test.ts
Formatting fs/empty_test.ts
...

After

> deno run prettier/main.ts test.ts
Formatting test.ts

In order to keep the original function unchanged, you should write like this

> deno run prettier/main.ts **/*test.ts
Formatting test.ts
Formatting fs/test.ts
Formatting fs/copy_test.ts
Formatting fs/empty_test.ts
...

Please share your thoughts and suggestions

@kt3k
Copy link
Member

kt3k commented May 22, 2019

npm has is-glob module. This looks a more robust approach to me.

source, test

@axetroy
Copy link
Contributor Author

axetroy commented May 22, 2019

@kt3k yes. that's great.

Are you interested in porting it?

@kt3k
Copy link
Member

kt3k commented May 22, 2019

@axetroy
I'm interested in it, but I can't start it right now. If you can do it, I appreciate your help!

@dshubhadeep
Copy link
Contributor

@axetroy I would like to work on it, if that's okay with you

@axetroy
Copy link
Contributor Author

axetroy commented May 22, 2019

@dshubhadeep
I appreciate your help.

expose a new function name isGlob

export function isGlob (str: string): boolean {
 // do the staff
}

@axetroy axetroy changed the title Do not use glob for prettier by default [discussion] Do not use glob for prettier by default May 22, 2019
@dshubhadeep
Copy link
Contributor

dshubhadeep commented May 22, 2019

@axetroy in which module should I add the function ? Btw the is-glob module also checks for extglob. Should that be added as well ?

@axetroy
Copy link
Contributor Author

axetroy commented May 22, 2019

@dshubhadeep
It should be added to fs module glob.ts file

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 a pull request may close this issue.

3 participants