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.

Sign up for GitHub

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

Jump to bottom

Reduce work to test if executable ends with a 'g'. #138

Merged
merged 1 commit into from
Jan 29, 2019
Merged

Reduce work to test if executable ends with a 'g'. #138

merged 1 commit into from
Jan 29, 2019

Conversation

elidoran
Copy link
Contributor

Avoid:

  • declaring path var
  • requiring path module
  • calling path.basename()
  • calling slice() to get one letter from the string

Instead:

  • refer to it from the argv array
  • refer to it as one less than the executable paths length

@elidoran
Reduce work to test if executable ends with a 'g'.
37d0b84 
Avoid:

* declaring path var
* requiring path module
* calling path.basename()
* calling slice() to get one letter from the string

Instead:

* refer to it from the argv array
* refer to it as one less than the executable paths length
@elidoran elidoran requested a review from a team as a code owner January 17, 2019 23:21
@aeschright aeschright added the semver:minor new backwards-compatible feature label Jan 22, 2019
zkat
zkat approved these changes Jan 28, 2019
View reviewed changes
Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

This... seems fine. I am very confused by the original implementation to this. This change should work just as well, thank you.

@elidoran
Copy link
Contributor Author

You’re welcome.
I was confused too. Maybe they had something else in mind way back in 2011?

@ljharb
Copy link
Contributor

ljharb commented Jan 29, 2019

That suggests the desire was to keep it working when the bin file was directly executed; iow, basename was removing the extension. Seems reasonable to either keep, or to discard?

@aeschright aeschright changed the base branch from latest to release-next January 29, 2019 23:28
@aeschright aeschright merged commit d9b6090 into npm:release-next Jan 29, 2019
aeschright pushed a commit that referenced this pull request Feb 14, 2019
@elidoran @aeschright
Reduce work to test if executable ends with a 'g'. (#138)
cb8707a 
PR-URL: #138
Credit: @elidoran
Reviewed-By: @aeschright
This was referenced May 1, 2022
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zkat zkat zkat approved these changes

Assignees
No one assigned
Labels
semver:minor new backwards-compatible feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@elidoran @ljharb @zkat @aeschright