Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

[Converge] path: refactor for performance and consistency #35

Closed
jasnell opened this issue May 22, 2015 · 8 comments
Closed

[Converge] path: refactor for performance and consistency #35

jasnell opened this issue May 22, 2015 · 8 comments

Comments

@jasnell
Copy link
Member

jasnell commented May 22, 2015

See: nodejs/node-v0.x-archive@c66f8c2
@misterdjules @woollybogger

Original commit message:

Improve performance by:
+ Not leaking the `arguments` object!
+ Getting the last character of a string by index, instead of
  with `.substr()` or `.slice()`

Improve code consistency by:
+ Using `[]` instead of `.charAt()` where possible
+ Using a function declaration instead of a var declaration
+ Using `.slice()` with clearer arguments
+ Checking if `dir` is truthy in `win32.format`
  (added tests for this)

Improve both by:
+ Making the reusable `trimArray()` function
+ Standardizing getting certain path statistics with
  the new `win32StatPath()` function
@Fishrock123
Copy link
Contributor

Maybe port to io.js, conflict isn't large.

@nwoltman
Copy link
Contributor

So should this commit be added to the io.js repo and not this new repo?

@Fishrock123
Copy link
Contributor

@woollybogger again, probably. If you'd be willing to do it, that would be great! :)

@jasnell
Copy link
Member Author

jasnell commented May 22, 2015

Eventually all of the io.js commits will make their way here anyway. So it
generally doesn't matter, especially for something like this.
On May 22, 2015 3:53 PM, "Jeremiah Senkpiel" [email protected]
wrote:

@woollybogger https://github.com/woollybogger again, probably. If you'd
be willing to do it, that would be great! :)


Reply to this email directly or view it on GitHub
nodejs/node#35 (comment).

@Fishrock123
Copy link
Contributor

While true, some of this is just good maintenance for the io.js line.

@jasnell
Copy link
Member Author

jasnell commented May 22, 2015

👍
On May 22, 2015 4:02 PM, "Jeremiah Senkpiel" [email protected]
wrote:

While true, some of this is just good maintenance for the io.js line.


Reply to this email directly or view it on GitHub
nodejs/node#35 (comment).

@jasnell
Copy link
Member Author

jasnell commented May 27, 2015

Closing this in favor of nodejs/node#1778

@jasnell jasnell closed this as completed May 27, 2015
@Fishrock123
Copy link
Contributor

Landed in io.js: nodejs/node#1778 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants