Skip to content
This repository has been archived by the owner on Mar 4, 2021. It is now read-only.

Adding missing NSString methods from NSPathUtilities.h #30

Closed
wants to merge 6 commits into from

Conversation

mauricio
Copy link
Contributor

These methods are part of Foundation (both on MacOS and iOS), but are declared at NSPathUtilities. They were not available on maccore and I need them to do some file name cleanups (that can't be done on System.IO as System.IO doesn't handle symlinks).

@mauricio
Copy link
Contributor Author

/cc @kumpera ;)

@kumpera
Copy link
Contributor

kumpera commented Jun 25, 2012

I'm not a maintainer of maccore, sorry. But let's review it!

PathComponents returns an array of strings, right? Why not change the return value to "string[]"? This would provide
a much nicer binding, wouldn't it? Mind you that this change could not match with the overall style of the given file, so
my suggestion can be wrong.

@mauricio
Copy link
Contributor Author

Yeah, that's what i thought first, but since there are many calls that do return NSArray I wasn't sure about what to do in this case. I'd surely change it and also change StringsByAppendingPaths() to accept an string[], since only strings should be given to this method.

@rscarvalho
Copy link

+1

1 similar comment
@thiagopnts
Copy link

+1

// methods from the category NSStringPathExtensions

[Export ("pathComponents")]
NSArray PathComponents();
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not return NSArray from maccore's public APIs, unless there is a strong reason to. We use strongly typed C# arrays

@migueldeicaza
Copy link
Contributor

I am actually confused about the comment for this commit.

These look purely like string operations on a path, it does not look like they would ever touch the file system, so I am confused as to why we would want these.

@mauricio
Copy link
Contributor Author

That's where the path handling methods are in Cocoa, they are (almost exclusively) in the NSString class. Given this is where people using Objective-C (or searching for these methods) are going to look for I think it's better if they are kept in the same place Objective-C shows them. Will do the other changes.

@mauricio
Copy link
Contributor Author

@migueldeicaza now I have made the fields that are properties in Objective-C to also be properties in C# (didn't know how to do that) and all methods now return either string[] or string.

These path utilites methods are needed (at least in our case) because they are the only way to "standarize" symlinks to their real paths, since there is no such concept at System.IO.Path and we need real full paths (and not symlinks) in our MonoMac app. Those are also methods that are available at any NSString instance, so I while it doesn't look like the best place for them, that's where Apple decided to place them (they're part of the well known class public API and I think both MonoTouch and MonoMac should cover as much of the public API as possible for these things).

@gaearon
Copy link

gaearon commented Jan 15, 2013

I wish stringByStandardizingPath was in MonoTouch. I'm not sure my string.Replace-s are correct.

@mauricio
Copy link
Contributor Author

@gaearon we just keep our own private branch here. It's unfortunate but it's necessary since it doesn't look like this will ever be accepted.

string[] PathComponents { [Bind("pathComponents")] get; }

[Export("isAbsolutePath")]
bool AbsolutePath { [Bind("isAbsolutePath")] get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Please rename "IsAbsolutePath"

@migueldeicaza
Copy link
Contributor

I am willing to get these changes, but there are a few things that need to change:

(a) there is a bug in the names referenced, so those need to be fixed.
(b) NSStrings should really return NSStrings in this case, even if our practice is to do "string"
(c) I hate the "stringBy..." method names. We should drop the "string" prefix, and just turn "stringByDeletingFoo" into "DeleteFoo"

@mauricio
Copy link
Contributor Author

There are too many other commits on this one, I'm going to create a new one with these changes since this branch has gone too far away from what this PR means.

@mauricio
Copy link
Contributor Author

New PR available at #42

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