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

Update #3 #5

Closed
wants to merge 9 commits into from
Closed

Update #3 #5

wants to merge 9 commits into from

Conversation

davecheney
Copy link
Contributor

  • Remove FileSystem.Join
  • Add Filesystem.PathSeparator
  • Add fs.Join package level func
  • Walker now uses Join(fs, ...)

Sorry about the diff, I have no idea how to rebase or any of that stuff

 * Remove FileSystem.Join
 * Add Filesystem.PathSeparator
 * Add fs.Join package level func
 * Walker now uses Join(fs, ...)
Conflicts:
	filesystem.go
	walk.go
@davecheney
Copy link
Contributor Author

Gentle ping.

@kr
Copy link
Owner

kr commented Nov 14, 2013

Nice, looking now.

Don't worry about the diff. I can clean it up.

sep := string(fs.PathSeparator())
for i, e := range elem {
if e != "" {
return filepath.Clean(strings.Join(elem[i:], sep))
Copy link
Owner

Choose a reason for hiding this comment

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

The behavior of filepath.Clean depends on the value of PathSeparator.
So I guess we have to copy Clean (and FromSlash, etc) into this package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't I do that with Clean(, sep) ?

On Fri, Nov 15, 2013 at 9:32 AM, Keith Rarick notifications@github.comwrote:

In filesystem.go:

@@ -33,4 +30,17 @@ func (f *fs) ReadDir(dirname string) ([]os.FileInfo, error) { return ioutil.Read

func (f *fs) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) }

-func (f *fs) Join(elem ...string) string { return filepath.Join(elem...) }
+func (f *fs) PathSeparator() byte { return os.PathSeparator }
+
+// Join joins any number of path elements into a single path, adding
+// a Separator if necessary. The result is Cleaned, in particular
+// all empty strings are ignored.
+func Join(fs FileSystem, elem ...string) string {

  • sep := string(fs.PathSeparator())
  • for i, e := range elem {
  •   if e != "" {
    
  •       return filepath.Clean(strings.Join(elem[i:], sep))
    

The behavior of filepath.Clean depends on the value of PathSeparator.
So I guess we have to copy Clean (and FromSlash, etc) into this package.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5/files#r7675907
.

Copy link
Owner

Choose a reason for hiding this comment

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

Didn't I do that with Clean(, sep) ?

Not sure what you mean. This line calls filepath.Clean,
but it needs to call a local copy of Clean, which isn't
in this package or this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pass the separator that we want to clean with into filepath.Clean.

I'll check again, I think it's doing the right thing, but I could very well
be wrong as I don't have a win32 machine to test on.

On Fri, Nov 15, 2013 at 9:57 AM, Keith Rarick notifications@github.comwrote:

In filesystem.go:

@@ -33,4 +30,17 @@ func (f *fs) ReadDir(dirname string) ([]os.FileInfo, error) { return ioutil.Read

func (f *fs) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) }

-func (f *fs) Join(elem ...string) string { return filepath.Join(elem...) }
+func (f *fs) PathSeparator() byte { return os.PathSeparator }
+
+// Join joins any number of path elements into a single path, adding
+// a Separator if necessary. The result is Cleaned, in particular
+// all empty strings are ignored.
+func Join(fs FileSystem, elem ...string) string {

  • sep := string(fs.PathSeparator())
  • for i, e := range elem {
  •   if e != "" {
    
  •       return filepath.Clean(strings.Join(elem[i:], sep))
    

Didn't I do that with Clean(, sep) ?

Not sure what you mean. This line calls filepath.Clean,
but it needs to call a local copy of Clean, which isn't
in this package or this pull request.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5/files#r7676624
.

Copy link
Owner

Choose a reason for hiding this comment

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

filepath.Clean doesn't take a separator.
It uses os.PathSeparator directly.

The separator here is for strings.Join.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn. I messed that up

On Fri, Nov 15, 2013 at 10:34 AM, Keith Rarick notifications@github.comwrote:

In filesystem.go:

@@ -33,4 +30,17 @@ func (f *fs) ReadDir(dirname string) ([]os.FileInfo, error) { return ioutil.Read

func (f *fs) Lstat(name string) (os.FileInfo, error) { return os.Lstat(name) }

-func (f *fs) Join(elem ...string) string { return filepath.Join(elem...) }
+func (f *fs) PathSeparator() byte { return os.PathSeparator }
+
+// Join joins any number of path elements into a single path, adding
+// a Separator if necessary. The result is Cleaned, in particular
+// all empty strings are ignored.
+func Join(fs FileSystem, elem ...string) string {

  • sep := string(fs.PathSeparator())
  • for i, e := range elem {
  •   if e != "" {
    
  •       return filepath.Clean(strings.Join(elem[i:], sep))
    

filepath.Clean doesn't take a separator.
It uses os.PathSeparator directly.

The separator here is for strings.Join.


Reply to this email directly or view it on GitHubhttps://github.com//pull/5/files#r7677532
.

@davecheney
Copy link
Contributor Author

I'd like to drop this.

@davecheney davecheney closed this Nov 15, 2013
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.

2 participants