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

--copy-ignored flag added to CLI #10887

Merged
merged 13 commits into from Jan 10, 2020
Merged

Conversation

@rajasekarm
Copy link
Member

rajasekarm commented Dec 18, 2019

Q                       A
Fixed Issues? Fixes #6226
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT
@rajasekarm rajasekarm added this to the v7.8.0 milestone Dec 18, 2019
@rajasekarm rajasekarm changed the title Reopen - Prevent ignored files in out dir Reopen - includeIgnored flag added to CLI Dec 18, 2019
@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 18, 2019

Reviews at #10831

if (
!written &&
((!isCompilableExtension && cliOptions.copyFiles) ||
cliOptions.includeIgnored)

This comment has been minimized.

Copy link
@JLHwung

JLHwung Dec 18, 2019

Contributor

I think we should throw in parseArgv when copyFiles: false, includeIgnored: true, otherwise we will have another flag that could copy non compilable files.

This comment has been minimized.

Copy link
@existentialism

existentialism Dec 18, 2019

Member

🚲🏠 warning:

Should we call it --copy-ignored? 😝

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 20, 2019

Author Member

Changed to --copy-ignored

@@ -0,0 +1,12 @@
{
"args": [

This comment has been minimized.

Copy link
@JLHwung

JLHwung Dec 18, 2019

Contributor

Can you add a test for --only?

babel src --out-dir lib --copy-files --only src/main/* --include-ignored

and a test when --include-ignored is not enabled (default of v7.8), so that we can make sure the old behaviour preserves.

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 20, 2019

Author Member

Is this still needed?

This comment has been minimized.

Copy link
@JLHwung

JLHwung Dec 23, 2019

Contributor

Good to have.😀

@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Dec 20, 2019

I'm throwing error if --copy-ignored is used without ignore flag. Which will solve other corner cases.

@rajasekarm rajasekarm requested review from existentialism and JLHwung Dec 20, 2019
@rajasekarm rajasekarm changed the title Reopen - includeIgnored flag added to CLI Reopen - --include-ignored flag added to CLI Dec 20, 2019
@nicolo-ribaudo nicolo-ribaudo removed the request for review from JLHwung Dec 20, 2019
@@ -1 +0,0 @@
a;

This comment has been minimized.

Copy link
@JLHwung

JLHwung Dec 22, 2019

Contributor

🤔Why is .foo.js not copied?

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 23, 2019

Author Member

we've --include-dotfile to Include dotfiles when compiling and copying non-compilable files.

https://github.com/rajasekarm/babel/blob/master/packages/babel-cli/src/babel/options.js#L148

This comment has been minimized.

Copy link
@JLHwung

JLHwung Dec 23, 2019

Contributor

Yeah, it was copied before this PR. But now .foo.js has been removed from the out files, which means it is not copied.

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 23, 2019

Author Member

I got it, I'll check.

This comment has been minimized.

This comment has been minimized.

Copy link
@nicolo-ribaudo

nicolo-ribaudo Dec 23, 2019

Member

The linked one is your master branch, not the PR branch 😛

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 24, 2019

Author Member

Yeah, I'm check this.

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 24, 2019

Author Member

Found the root cause of this bug, I'm fixing it.

This comment has been minimized.

Copy link
@rajasekarm

rajasekarm Dec 25, 2019

Author Member

.foo.js is inside src/foo and it is in ignored list. So it is not copied.

@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Dec 25, 2019

I've exposed pathToPattern function from @babel/core for the ignore file check.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 25, 2019

I've exposed pathToPattern function from @babel/core for the ignore file check.

I have bad news for you 😛
This is a breaking change because if someone used @babel/core v7.7.7 and updates @babel/cli to 7.8.0 won't work.
For now, we can duplicate the code (with comments to explain that it's duplicated and if it is updated it should be done in both places) and then revisit it for Babel 8.

@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Dec 25, 2019

Sure, I'll make the change.

@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Dec 25, 2019

@nicolo-ribaudo Done 👍

@nicolo-ribaudo nicolo-ribaudo changed the title Reopen - --include-ignored flag added to CLI Reopen - --copy-ignored flag added to CLI Dec 25, 2019
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review Dec 25, 2019

(I need to review this PR again)

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Dec 25, 2019

I was looking at the write function in packages/babel-cli/src/babel/dir.js. From what I understood, it can return false in three cases:

  1. The extension is not compatible (first return false)
  2. The file has been ignored, and thus babel.transformSync (aka util.compile) returns null
  3. There was an error.

If we make the write function return a string enum rather than a boolean, maybe we could avoid checking if the file has been ignored? Also, I think that the current approach doesn't work well if a file has been ignored not using a cli option, but using a config file.

@rajasekarm rajasekarm changed the title Reopen - --copy-ignored flag added to CLI copy-ignored flag added to CLI Dec 27, 2019
@rajasekarm rajasekarm changed the title copy-ignored flag added to CLI --copy-ignored flag added to CLI Dec 27, 2019
@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Dec 27, 2019

@nicolo-ribaudo I made the requested change. Can you please check?

Copy link
Member

nicolo-ribaudo left a comment

The code now looks way cleaner!

@nicolo-ribaudo nicolo-ribaudo requested a review from JLHwung Dec 27, 2019
@JLHwung

This comment has been minimized.

Copy link
Contributor

JLHwung commented Dec 31, 2019

@rajasekarm Can you rebase on upstream master? 🤔It should not have legacy CircleCI test check here.

@rajasekarm rajasekarm force-pushed the rajasekarm:fix/10829 branch from 5ee1ead to f6e1963 Dec 31, 2019
Copy link
Member

kaicataldo left a comment

This code is so much clearer 🎉

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Jan 6, 2020

@rajasekarm Could you open a PR to babel/website for the docs?

@rajasekarm

This comment has been minimized.

Copy link
Member Author

rajasekarm commented Jan 6, 2020

@nicolo-ribaudo I'll do it tonight.

@nicolo-ribaudo nicolo-ribaudo merged commit 8415065 into babel:master Jan 10, 2020
4 of 5 checks passed
4 of 5 checks passed
build (13.x)
Details
test262 Workflow: test262
Details
Travis CI - Pull Request Build Passed
Details
build-standalone Workflow: build-standalone
Details
e2e Workflow: e2e
Details
@rajasekarm rajasekarm deleted the rajasekarm:fix/10829 branch Jan 10, 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.

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