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

Expand directories in CLI (2020) #7660

Merged
merged 14 commits into from Mar 16, 2020
Merged

Expand directories in CLI (2020) #7660

merged 14 commits into from Mar 16, 2020

Conversation

@thorn0
Copy link
Collaborator

thorn0 commented Feb 23, 2020

Closes #6085
Supersedes #6128
Supersedes #6285

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@thorn0 thorn0 added the status:wip label Feb 23, 2020
@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch 3 times, most recently from 93755e0 to e26bd2f Feb 23, 2020
@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 23, 2020

Resolved files aren't globally sorted anymore. Instead, their order matches the order of the given patterns, and for each pattern, the corresponding group of files is sorted. This seems logical and acceptable while ditching the global sorting allows us to use generators to avoid various problems related to big arrays.

Open problems:

  1. files found by globs but all ignored by .prettierignore - warning?
  2. directory expanding + --parser: what is "supported files" then?
  3. directory expanding + glob-based config overrides: same question
@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch 2 times, most recently from 2a38cf6 to 5637b04 Feb 24, 2020
@thorn0 thorn0 removed the status:wip label Feb 24, 2020
@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch 2 times, most recently from d570c01 to c896ba5 Feb 24, 2020
@thorn0 thorn0 mentioned this pull request Feb 24, 2020
0 of 3 tasks complete
@thorn0 thorn0 added this to the 2.0 milestone Feb 24, 2020
@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch from b849ca7 to 314ca20 Feb 25, 2020
src/cli/expand-patterns.js Outdated Show resolved Hide resolved
.map(ext => "*" + (ext[0] === "." ? ext : "." + ext))
.concat(filenames)}}`;
}
return supportedFilesGlob;

This comment has been minimized.

Copy link
@fisker

fisker Feb 25, 2020

Collaborator

Can I format exts add by config override or plugins? I think we should glob all files
and inferParser then filter the files without parser

This comment has been minimized.

Copy link
@thorn0

thorn0 Feb 25, 2020

Author Collaborator

Can I format exts add by config override or plugins?

Plugins - yes. There is a test for that.

Config overrides - no. Because configs are resolved on a later stage.

I think we should glob all files and inferParser then filter the files without parser

That's a good idea. I'll give it a try.

This comment has been minimized.

Copy link
@thorn0

thorn0 Feb 26, 2020

Author Collaborator

This parser inference thing looks a bit complicated though. How about merging this PR as is and improving this later?

I feel this PR is already good enough for most of the use cases. People just want to run prettier . --write or prettier src --write. For more complicated scenarios, they can use globs.

Copy link
Member

evilebottnawi left a comment

Good job! Couple notes

tests_integration/__tests__/patterns-dirs.js Outdated Show resolved Hide resolved
src/cli/expand-patterns.js Outdated Show resolved Hide resolved
src/cli/expand-patterns.js Outdated Show resolved Hide resolved
for (const pattern of context.filePatterns) {
const absolutePath = path.resolve(cwd, pattern);

if (containsIgnoredPathSegment(absolutePath, cwd, ignoredDirectories)) {

This comment has been minimized.

Copy link
@evilebottnawi

evilebottnawi Feb 26, 2020

Member

What about using micromatch here, we already have it in our deps (from fast-glob)

This comment has been minimized.

Copy link
@thorn0

thorn0 Feb 26, 2020

Author Collaborator

This code is only for silent ignoring files in node_modules and VCS dirs. I think it's good enough.

@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch 5 times, most recently from e0b374d to ae238af Feb 26, 2020
@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 26, 2020

I guess only problem is how to get supported files in dir?

@thorn0

@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 26, 2020

Can you also post a log run ./bin/prettier.js . on codebase?

@thorn0 thorn0 force-pushed the thorn0:expand-dirs branch from a853b1b to a3ef04a Feb 27, 2020
@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 27, 2020

I forgot that when I was thinking about this feature, I was going to do something like glob all files from dir and yield {file, ignoreNoParserError: true} , deal with it later in handleError. This can handle unknown files add by config overrides.

It seems if we do this, the NoResultsError also need move there, I didn't think about this before.

@lydell

This comment has been minimized.

Copy link
Collaborator

lydell commented Feb 27, 2020

What are the answers to the open problems?

Open problems:

  1. files found by globs but all ignored by .prettierignore - warning?
  2. directory expanding + --parser: what is "supported files" then?
  3. directory expanding + glob-based config overrides: same question
Copy link
Member

evilebottnawi left a comment

Good job

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

What are the answers to the open problems?

# 2 and # 3 are the same problem, parser inference vs using extensions.

As for # 1, Prettier 1.19.1 doesn't issue any warning if passed a glob that is fully ignored by .prettierignore. Maybe it's not a big deal if the same happens with directories.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 27, 2020

@thorn0

directory expanding + --parser: what is "supported files" then?

Maybe we should using only parser extensions for better DX and 0cjs

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

@evilebottnawi

directory expanding + --parser: what is "supported files" then?

Maybe we should using only parser extensions for better DX and 0cjs

I'd rather expect that prettier dir --parser html would match dir/**/* and use html for all files.

@fisker

glob all files from dir and yield {file, ignoreNoParserError: true} , deal with it later in handleError. This can handle unknown files add by config overrides. It seems if we do this, the NoResultsError also need move there, I didn't think about this before.

Yes. It's tricky. Not super difficult, but I'm sure that if I really start digging into it, I'll discover a dozen of new microissues there, so it might take long. The .prettierignore part looks especially suspicious. You tried to update the ignore package, so you should have an idea of what I mean.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 27, 2020

I'd rather expect that prettier dir --parser html would match dir/**/* and use html for all files.

Yes, i think you are right. Do we have a test for this?

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Feb 27, 2020

I'd rather expect that prettier dir --parser html would match dir/**/* and use html for all files.

This should work exactly if we used dir/**/*, so you're right. It should just follow the current precedence order: command line args / config via API, config file (overrides and whatnot), inference based on extension.

IMO these things shouldn't block this PR if these problems already existed when using a blob, but those problems were things I wished to fix for a very long time (that would end up being a 2.0 release), that are way more important than other things people are more concerned with 😅

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

I'd rather expect that prettier dir --parser html would match dir/**/* and use html for all files.

Yes, i think you are right. Do we have a test for this?

That's not how it works now. It's the same issue, namely: what exactly is "supported files" when we're searching for all "supported files" in a directory?

The current implementation in this PR: "supported files" = files with extensions and names taken from getSupportInfo().

An ideal implementation: "supported files" = glob(dir + '/**').filter(parserCanBeInferred). In the case when --parser is specified as a CLI arg, parserCanBeInferred turns into () => true.

@duailibe

This comment has been minimized.

Copy link
Member

duailibe commented Feb 27, 2020

Ah, now I understand the problem

The current implementation in this PR: "supported files" = files with extensions and names taken from getSupportInfo().

This sounds good to me

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

My proposal is to postpone the ideal implementation until 3.0.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Feb 27, 2020

My proposal is to postpone the ideal implementation until 3.0.

Sounds good for me, we will also collect feedback and it will help us make it better

@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 27, 2020

Looks like @duailibe and @evilebottnawi both Okay with current implementation, I'm ok with it too.

But I have a question, can we somehow hide this NoParserError for both dir and glob? I'm on cellphone, I don't know how ESLint handle eslint foo.js *.jpg bar.js? I guess it throw a ParseError and abort lint progress?

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

@fisker Currently (in 1.19.1 and in this PR), the "parser can't be inferred" error is not fatal. The error is logged, the file is skipped, Prettier CLI proceeds with next file. Why would we want to hide the error? ESLint doesn't have relevant behavior because it always uses the same parser, but I don't think it ever aborts the whole linting run because of a single file.

@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 27, 2020

@thorn0 I was thinking if eslint abort linting when error occurs, we can also ask people pass glob with valid extensions like prettier *.{js,css} instead of prettier *. For ESLint I never use --ext, I use **/*.{js,mjs}

Update: If eslint ignore error, we can also ignore it , so we don't need find "supported files"

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

@fisker Not sure how what you wrote this time is different from what you wrote before about 'I was going to do something like glob all files from dir and yield {file, ignoreNoParserError: true}'.

@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Feb 27, 2020

I'm okay with current implementation.

This is only a idea of new possible/simple solution.

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Feb 27, 2020

I'm actually trying to rewrite it. Let's see how it goes. If it works, I'll open a new PR.

@fisker

This comment has been minimized.

Copy link
Collaborator

fisker commented Mar 10, 2020

Should we merge this and release v2.0?

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Mar 11, 2020

/cc @prettier/core Any review will be helpful.

Copy link

dougal83 left a comment

Looks good. Great work. Could open a new PR to refactor at this point.

@evilebottnawi

This comment has been minimized.

Copy link
Member

evilebottnawi commented Mar 16, 2020

/cc @thorn0 Can we merge it and start to release next version?

@thorn0

This comment has been minimized.

Copy link
Collaborator Author

thorn0 commented Mar 16, 2020

@evilebottnawi Okay, I'll do this today (if nobody objects).

@lipis
lipis approved these changes Mar 16, 2020
@thorn0 thorn0 merged commit 3f1c463 into prettier:next Mar 16, 2020
38 checks passed
38 checks passed
Dev test on node 13 and ubuntu-latest
Details
Lint
Details
Build
Details
Dev test on node 12 and ubuntu-latest
Details
Dev test on node 10 and ubuntu-latest
Details
Dev test on node 13 and macos-latest
Details
Dev test on node 12 and macos-latest
Details
Dev test on node 10 and macos-latest
Details
Dev test on node 13 and windows-latest
Details
Dev test on node 12 and windows-latest
Details
Dev test on node 10 and windows-latest
Details
Prod test on node 13 and ubuntu-latest
Details
Prod test on node 12 and ubuntu-latest
Details
Prod test on node 10 and ubuntu-latest
Details
Prod test on node 13 and macos-latest
Details
Prod test on node 12 and macos-latest
Details
Prod test on node 10 and macos-latest
Details
Prod test on node 13 and windows-latest
Details
Prod test on node 12 and windows-latest
Details
Prod test on node 10 and windows-latest
Details
Header rules No header rules processed
Details
Pages changed All files already uploaded
Details
Mixed content No mixed content detected
Details
Redirect rules 4 redirect rules processed
Details
codecov/patch 94.56% of diff hit (target 80%)
Details
codecov/project 94.38% (+<.01%) compared to 1f64401
Details
deploy/netlify Deploy preview ready!
Details
prettier.prettier Build #20200227.2 succeeded
Details
prettier.prettier (Dev Lint on Linux Node v12) Dev Lint on Linux Node v12 succeeded
Details
prettier.prettier (Dev Test on Linux Node v12) Dev Test on Linux Node v12 succeeded
Details
prettier.prettier (Dev Test on Windows Node v12) Dev Test on Windows Node v12 succeeded
Details
prettier.prettier (Dev Test on macOS Node v12) Dev Test on macOS Node v12 succeeded
Details
prettier.prettier (Prod Build on Linux Node v12) Prod Build on Linux Node v12 succeeded
Details
prettier.prettier (Prod Lint on Linux Node v12) Prod Lint on Linux Node v12 succeeded
Details
prettier.prettier (Prod Pack on Linux Node v12) Prod Pack on Linux Node v12 succeeded
Details
prettier.prettier (Prod Test on macOS Node_v10) Prod Test on macOS Node_v10 succeeded
Details
prettier.prettier (Prod Test on macOS Node_v12) Prod Test on macOS Node_v12 succeeded
Details
prettier.prettier (Prod Test on macOS Node_v12_standalone) Prod Test on macOS Node_v12_standalone succeeded
Details
thorn0 added a commit that referenced this pull request Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.