Skip to content

Conversation

@Dominator008
Copy link
Contributor

Fixes #1407.
Fixes #1477.

@ChadKillingsworth
Copy link
Collaborator

I can test this tonight, but I don't have a windows build environment at the office.

@Dominator008
Copy link
Contributor Author

@ChadKillingsworth This is passing on my Windows environment.

@ChadKillingsworth
Copy link
Collaborator

Yeah I just meant if we wanted someone else to try it out - since Travis can't build on Windows.

@Dominator008
Copy link
Contributor Author

@supersteves Can you help verifying this? Thanks!

@ChadKillingsworth
Copy link
Collaborator

@Dominator008 Did you see the comment by @joeltine on #1407? Does that pattern work with this change?

@Dominator008
Copy link
Contributor Author

@ChadKillingsworth No it doesn't seem to be working :(

@Dominator008
Copy link
Contributor Author

@ChadKillingsworth OK the regression should be fixed now.

@Dominator008 Dominator008 changed the title Fix glob on Windows Fix glob on Windows and fix regression Feb 1, 2016
@supersteves
Copy link
Contributor

Fix verified 👍
(strange how Ant failed but Mvn hung, though - but both working now)

@Dominator008
Copy link
Contributor Author

Thanks @supersteves !

@dimvar
Copy link
Contributor

dimvar commented Feb 1, 2016

@Dominator008 can you add a unit test for the --js='!**\./node_modules**.js' pattern?

@dimvar
Copy link
Contributor

dimvar commented Feb 1, 2016

Other than that lgtm

Dominator008 referenced this pull request Feb 2, 2016
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=113553220
@Dominator008
Copy link
Contributor Author

@dimvar The unit test for --js='!**\./foo**.js' requires creating a temporary directory under the current working directory (see testGlobJs5). This works on my machine, but I'm not sure if it will work internally since we need to have write permission on the current working directory.

@dimvar
Copy link
Contributor

dimvar commented Feb 2, 2016

OK sounds good. LGTM then. I'll look to see if I can add a unit test internally after this lands. I'll merge this once Travis is done.

@dimvar
Copy link
Contributor

dimvar commented Feb 2, 2016

@Dominator008 looks like the travis build failed. Can you look into it so we can merge the PR?

Also fix regression on patterns like '!**\./node_modules**.js'
introduced by google#1409 (dfd1853).

Fixes google#1407.
Fixes google#1477.
@Dominator008
Copy link
Contributor Author

@dimvar The Travis build failed for other reasons and I fixed them in #1486. I've rebased this on top of master. Let's see if this one succeeds :)

@dimvar
Copy link
Contributor

dimvar commented Feb 2, 2016

Thanks! Feel free to merge this once Travis is done.

Dominator008 added a commit that referenced this pull request Feb 2, 2016
Fix glob on Windows and fix regression
@Dominator008 Dominator008 merged commit a3ce52a into google:master Feb 2, 2016
@Dominator008 Dominator008 deleted the glob branch February 2, 2016 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants