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

relative() and isWithin() are not case insensitive on Windows #14

Closed
munificent opened this issue Sep 29, 2016 · 5 comments
Closed

relative() and isWithin() are not case insensitive on Windows #14

munificent opened this issue Sep 29, 2016 · 5 comments
Assignees
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@munificent
Copy link
Member

This:

import 'package:path/path.dart' as p;

main() {
  print(p.windows.isWithin(r"a\b", r"a\B\c"));
  print(p.windows.isWithin(r"a:b\c", r"A:\b\c\d"));
  print(p.windows.relative(r"a:\b\c\d", from: r"a:\B\c"));
  print(p.windows.relative(r"a:\b\c\d", from: r"A:\b\c"));
}

Prints:

false
false
..\..\b\c\d
d

The first three are wrong. Interestingly, the last is right!

So isWithin() is not handling case sensitivity of drive letters or paths. relative() is handling drive letters, but not paths.

@munificent munificent added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Sep 29, 2016
@nex3
Copy link
Member

nex3 commented Sep 29, 2016

This also raises the question of how to handle normalize(). Currently, normalize() is the best way to create a map that uses path keys that doesn't count textually-different paths as identical (watcher does this, for example). In order to make that safe, we'd have to make normalize() downcase paths on Windows as well.

Normalization is also expected to make a path more human-readable though, and wiping the case makes it less readable. So maybe the thing to do is add p.equals() and p.hash() methods that users can pass to new HashMap() to create a proper path map (or set).

@munificent
Copy link
Member Author

So maybe the thing to do is add p.equals() and p.hash() methods that users can pass to new HashMap() to create a proper path map (or set).

+1 for equals().

I'm not crazy about hash() since I think users might expect that to return the actual hash code and not a string-which-can-be-hashed-to-get-a-good-hash-code.

Maybe canonicalize, or hashNormalize?

@nex3
Copy link
Member

nex3 commented Sep 30, 2016

I'm not crazy about hash() since I think users might expect that to return the actual hash code and not a string-which-can-be-hashed-to-get-a-good-hash-code.

The intention is for it to return the hash code, so it can be passed into new HashMap(). We could also add canonicalize() if you think it would be useful, although I can't come up with a use-case off the top of my head.

@munificent
Copy link
Member Author

The intention is for it to return the hash code, so it can be passed into new HashMap().

Ah, I see. Yeah, that could work, though I feel HashMap() is pretty far off the beaten path. My hunch is most users would intuitively expect to use a vanilla map and just canonicalize their path strings such that equivalent paths become equivalent strings.

@nex3
Copy link
Member

nex3 commented Sep 30, 2016

We can totally add canonicalize() then—it's certainly easy enough to implement. Maybe we can point the user to HashMap() for more efficiency, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

2 participants