Skip to content

--files flag windows support#30222

Merged
samouri merged 3 commits intoampproject:masterfrom
samouri:fix-files-windows
Sep 14, 2020
Merged

--files flag windows support#30222
samouri merged 3 commits intoampproject:masterfrom
samouri:fix-files-windows

Conversation

@samouri
Copy link
Copy Markdown
Member

@samouri samouri commented Sep 14, 2020

summary
Globby does not support windows paths.
This fix worked for --flags, but we may want to look into switching to a lib that explicitly cares about Windows support.

@samouri samouri marked this pull request as ready for review September 14, 2020 17:12
@samouri samouri requested a review from rsimha September 14, 2020 17:12
@amp-owners-bot amp-owners-bot bot requested a review from rcebulko September 14, 2020 17:12
Copy link
Copy Markdown
Contributor

@rsimha rsimha left a comment

Choose a reason for hiding this comment

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

Agree with both the fix and the TODO.

@samouri
Copy link
Copy Markdown
Member Author

samouri commented Sep 14, 2020

Such a speedy review! Thanks @rsimha

@samouri samouri self-assigned this Sep 14, 2020
Copy link
Copy Markdown
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Agree with all above 👍

Comment on lines +100 to +103
argv.files
.split(',')
.map((s) => s.trim())
.map(toPosix)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: We can run this once instead of n:

Suggested change
argv.files
.split(',')
.map((s) => s.trim())
.map(toPosix)
toPosix(argv.files)
.split(/\s*,\s*/)

Copy link
Copy Markdown
Member Author

@samouri samouri Sep 14, 2020

Choose a reason for hiding this comment

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

I'm mildly ashamed to admit I don't know how this works 😬

Copy link
Copy Markdown
Member Author

@samouri samouri Sep 14, 2020

Choose a reason for hiding this comment

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

Oh wait, I get it. Thats super clever. I didn't know .split accepted regexes. Not going to switch from a readability standpoint...using my own confusion as a litmus test :)

@samouri samouri merged commit a670e5c into ampproject:master Sep 14, 2020
@samouri samouri deleted the fix-files-windows branch September 14, 2020 22:06
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* --files flag windows support

* Update utils.js

* lint-rollin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants