Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Optional appropriate environ replace #93

Closed
wants to merge 1 commit into from

Conversation

gucong3000
Copy link
Contributor

@gucong3000 gucong3000 commented Nov 9, 2018

By this PR, we can use a cross-platform configuration items in the .npmrc file:

shell=${GIT_INSTALL_ROOT-}/bin/bash
script-shell=${GIT_INSTALL_ROOT-}/bin/sh

@gucong3000 gucong3000 requested a review from a team as a code owner November 9, 2018 09:26
@zkat zkat added semver:minor new backwards-compatible feature needs-discussion labels Nov 13, 2018
@aeschright
Copy link
Contributor

This needs tests and some documentation to explain what the feature is and how it works.

Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

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

Bug in the \ handling logic, and definitely needs a test and update to the docs.

esc = failbackMatch[1]
esc = esc.length && esc.length % 2
if (esc) continue
failback = name.slice(failbackMatch.index + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have an even number of \ chars, then this will be the wrong offset. https://gist.github.com/isaacs/c2b12db30a49324325a3781302668408

I recommend writing the failback parsing logic as a new npm module, so that it can get proper tests and documentation.

@darcyclarke
Copy link
Contributor

@gucong3000 appreciate the contribution; That said, this seems to have some caveats to the implementation & looks fairly nuanced in terms of usage. I'm going to close this for now.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants