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

[SR-10048] NSRange.init?(_ string: String) should either be internal or named to reflect its purpose #3521

Open
swift-ci opened this issue Mar 5, 2019 · 6 comments

Comments

@swift-ci
Copy link
Contributor

swift-ci commented Mar 5, 2019

Previous ID SR-10048
Radar None
Original Reporter benpious (JIRA User)
Type Bug
Environment

Xcode 10.1.

Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: e1582cb13c604afacfd80b0d7cf9f719

Issue Description:

Based on the function signature, a caller might reasonably expect [this function|https://github.com/apple/swift-corelibs-foundation/blob/master/Foundation/NSRange.swift#L150|http://example.com] to take a string, and return a range that encompasses the entire string. It returning nil seems a little strange, but it seems reasonable that it might return nil if the string is empty.

In fact, this initializer does something related to finding decimal numbers?

This init shows up in autocomplete in Xcode, and can create confusion for users. It should either be private, have a name that makes sense, have documentation, or some combination of the three.

@belkadan
Copy link

cc @airspeedswift, @phausler

@phausler
Copy link
Member

This just needs docs. It is the moral replacement to NSRangeFromString

@airspeedswift
Copy link
Member

I think it needs more than a documentation change. It needs a clarifying argument label. We should deprecate and replace it with an init that takes one.

@belkadan
Copy link

We don't have an argument label for going from Int to String either. This is a LosslessStringConvertible sort of thing.

@swift-ci
Copy link
Contributor Author

Comment by Ben Pious (JIRA)

I agree that a clarifying label would be nice.

@belkadan I think this is distinct from the similar function you mentioned in several respects:

This function does not fulfill the same promises as LosslessStringConvertible: that you can go from A -> B -> A without ambiguity. `NSRange(NSRange(location: -1, length: 4).description) == NSRange(location: 1, length: 4)`. The negative sign is lost. Perhaps that's just a bug, but still notable imo.

This is also far more liberal about what it accepts than Int?(_: String), for example. "3a" can't be converted to an Int by that function, but "3 a 4" can be converted to an NSRange.

Incidentally, this function doesn't match the behavior of NSRangeFromString in an important respect: it rejects Strings like "1", while according to the documentation of NSRangeFromString this should be equivalent to calling NSRange(1, 0).

Has any consideration been given to just removing this? I'm actually quite curious what the rationale for adding this to the original Core Foundation type was, since it seems like a bad idea to have something that's this liberal about what it accepts.

@phausler
Copy link
Member

The original API proposal that went through the internal Cocoa API review boiled down to: this is a safer alternative than the free floating function that did the same thing (NSRangeFromString) which definitely has consumers for archival. The C implementation would annoyingly throw NSExceptions and I found it much more ergonomic to use a nullable return instead. Changing this could be viewed as unproductive churn especially since we are advocating using Range instead since it has a better generic story than NSRange especially when it comes to strings. That all being said archival is important for end user data like document formats even if they are “legacy” so I am hesitant to advocate removing this entirely and pushing developers to bridge out to objc just to handle stringified NSRanges. In short, if we get a substantial glut of refinements for Foundation then I could see this being lumped in with those as a internal API proposal but just on its own it seems like small pickings.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
@shahmishal shahmishal transferred this issue from apple/swift May 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants