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

[Do not pull] [stdlib] SE-0132 renamings #3793

Closed
wants to merge 7 commits into from
Closed

[Do not pull] [stdlib] SE-0132 renamings #3793

wants to merge 7 commits into from

Conversation

beccadax
Copy link
Contributor

@beccadax beccadax commented Jul 27, 2016

What's in this pull request?

This pull request renames the ten methods listed in the under-review proposal SE-0132. (There are only ten because drop(while:) has not yet been implemented.)

It includes:

  • Renaming of the ten methods specified in the "Sequence-end operations" section of the detailed design.
  • Renaming of related private symbols, like _DropFirstSequence and _customIndexOfEquatableElement.
  • Stub methods at the old names with @available attributes to produce correct fix-its.
  • Updates to existing tests so that they use the new names.
  • Updates to documentation.

It does not include:

  • Incomplete ranges, the RangeExpression protocol, or the new range operators. Those are all in [Do not merge] [stdlib] IncompleteRange prototype #3737.
  • Removal of prefix(upTo:), prefix(through:), or suffix(from:). (Note that this is not in either pull request.)
  • Tests of the new name stubs.

Outstanding issues:

  • I have included stubs for some names, like index(of:), which have not shipped in a release version of Swift but have been in development versions of Swift since the API Guidelines were adopted. You may not want to keep these.
  • I have not attempted to merge the changes in [Do not merge] [stdlib] IncompleteRange prototype #3737 with this pull request. I suspect there will be minor conflicts in the documentation, but I don't think there will be anything that's too difficult to resolve.
  • I am still running the long tests on my Mac, so I can't be sure this passes the entire test suite.
  • SE-0132 has not been approved and may still be rejected or modified.

Since the deadline is close, it's worth noting that I probably won't be available tomorrow until the early afternoon. If minor changes are needed (for instance, the core team changes some of the names in the proposal), it may be best to make the final changes to this pull request yourselves instead of waiting for me.

CC: @lattner @dabrahams


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@beccadax
Copy link
Contributor Author

One other issue, actually: NSArray's -indexOfObject: enters Swift as index(of:); should that now be firstIndex(of:)? If the answer is "yes", I don't know how to implement that.

@dabrahams
Copy link
Contributor

on Wed Jul 27 2016, Brent Royal-Gordon <notifications-AT-github.com> wrote:

One other issue, actually: NSArray's -indexOfObject: enters Swift as
index(of:); should that now be firstIndex(of:)? If the answer is
"yes", I don't know how to implement that.

The answer is probably “yes,” but it's up to the Foundation team. You'd
implement it in the Foundation overlay and in API notes, but don't worry
about it if you can't figure that out. We can ask the Foundation team
to take care of it.

-Dave

@beccadax
Copy link
Contributor Author

With the just-pushed changes, this now passes every test I know how to invoke on my Mac.

@beccadax
Copy link
Contributor Author

beccadax commented Jul 27, 2016

Looking more closely at the Foundation issue:

  • Where index(of:) goes, so should index(of:in:), most likely.
  • There are other index(of:) methods in Foundation and elsewhere: OrderedSet, NSPasteboard, NSMenuItem. However, they all appear to apply to collections which either can't or shouldn't contain duplicates, so the first isn't critical.
  • Photos.framework's PHFetchResult may be an exception; it specifically offers "the same methods and conventions used by the NSArray class".
  • However, none of these are return-type-compatible with Collection, because they use NSNotFound returns instead of nil.
  • And in fact, none of these seem to actually conform to Collection yet at all.

I'm going to punt on this; these are technically out of the proposal's scope, and the decisions feel like they're a bit above my pay grade. (But if you do decide it should be done, I can probably do it; the API notes and overlays don't look difficult.)

@CodaFi
Copy link
Contributor

CodaFi commented Sep 22, 2017

@brentdax With rebasing, this counts as an implementation if you want to revive discussion of SE-0132.

beccadax added a commit to beccadax/swift-evolution that referenced this pull request Nov 16, 2017
@beccadax
Copy link
Contributor Author

@CodaFi Rebasing turned out to be, uh, challenging, but I've now reimplemented the proposal on top of a recent version of master. I'm not sure I'm using the @available attributes the way I should (see commit 64de85c for the clearest examples of what I'm doing), so comments are welcome.

@stephentyrone
Copy link
Contributor

What's the status of this? Should it be closed? Does it need attention from someone specific to proceed?

@beccadax
Copy link
Contributor Author

beccadax commented Apr 4, 2018

I'm closing this since even a revised version of the proposal would be easier to implement from scratch than to update at this point.

@beccadax beccadax closed this Apr 4, 2018
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.

4 participants