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.

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

Revert "path: resolve normalize drive letter to lower case" #100

Closed
wants to merge 1 commit into from
Closed

Revert "path: resolve normalize drive letter to lower case" #100

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Dec 6, 2014

This fixes realpath, but test-fs-realpath still fails due to a different issue. Looking into it.

See #17.

@bnoordhuis
Copy link
Member

The commit log should explain why it reverts f6e5740, i.e. why the change in that commit is wrong.

@bnoordhuis
Copy link
Member

@piscisaureus Can you take a look?

This reverts commit f6e5740.

Changing drive letters to lowercase violates the principle of
least surprise. Other functions that do this should get fixed too.

Conflicts:
	lib/path.js
@seishun
Copy link
Contributor Author

seishun commented Dec 6, 2014

I'm not sure how much of the backstory is should include. I've added a short rationale for now.

@bnoordhuis
Copy link
Member

Thanks, @seishun.

@piscisaureus PTAL. If this was discussed somewhere (which I believe it was), can you add a link?

@rvagg
Copy link
Member

rvagg commented Dec 6, 2014

@seishun do the tests enforce this behaviour or do we need a new explicit test to lock it in place?

@seishun
Copy link
Contributor Author

seishun commented Dec 6, 2014

@rvagg the tests are already case-sensitive, I don't think they could do any better.

By the way, the test still fails with this change due to path.join() changing the case of the drive letter (see nodejs/node-v0.x-archive#7031) due to the path.normalize() change mentioned in node-forward/node#20. Should I go ahead and submit a PR fixing that?

@caineio
Copy link

caineio commented Dec 8, 2014

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

  1. Which part of core do you think it might be related to?
    One of: debugger, http, assert, buffer, child_process, cluster, crypto, dgram, dns, domain, events, fs, http, https, module, net, os, path, querystring, readline, repl, smalloc, stream, timers, tls, url, util, vm, zlib, c++, docs, other (label)
  2. Which versions of io.js do you think are affected by this?
    One of: v0.10, v0.12, v1.0.0 (label)
  3. PR-only Does make test pass after applying this Pull Request.
    Expected: yes
  4. PR-only Is the commit message properly formatted? (See
    CONTRIBUTING.md for more information)
    Expected: yes

Please provide the answers in an ordered list like this:

  1. Answer for the first question
  2. Answer for the second question
  3. ...

Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say: ...summoning the core team devs!.

In case of validation problem I will say: Sorry, but something is not right here:.

Truly yours,
Caine.

Responsibilities

  1. indutny: crypto, tls, https, child_process, c++
  2. trevnorris: buffer, http, https, smalloc
  3. bnoordhuis: http, cluster, child_process, dgram

piscisaureus pushed a commit that referenced this pull request Dec 8, 2014
This reverts commit f6e5740.

Changing drive letters to lowercase violates the principle of
least surprise. Other functions that do this should get fixed too.

Conflicts:
	lib/path.js

PR-URL: #100
Reviewed-By: Bert Belder <[email protected]>
@piscisaureus piscisaureus self-assigned this Dec 8, 2014
piscisaureus pushed a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
Changing drive letters to lowercase violates the principle of
least surprise.

PR-URL: nodejs/node#100
piscisaureus added a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 15, 2015
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants